Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-114505: Add missing header file dependencies #114513

Merged
merged 5 commits into from
Feb 7, 2024
Merged

gh-114505: Add missing header file dependencies #114513

merged 5 commits into from
Feb 7, 2024

Conversation

smontanaro
Copy link
Contributor

@smontanaro smontanaro commented Jan 24, 2024 •
edited by bedevere-app bot
Loading

@colesbury and @erlend-aasland pointed out problems with some header file dependencies in Makefile.pre.in. This PR corrects the obvious problems, the definition of PYTHON_HEADERS and the dependencies for Programs/_testembed.o.

I believe the various *_HEADERS definitions are now correct (I found nothing else amiss besides PYTHON_HEADERS). I've not yet looked at any other .o -> .h dependencies, but will do that.

@bedevere-app bedevere-app bot mentioned this pull request Jan 24, 2024
Closed
@erlend-aasland erlend-aasland self-assigned this Jan 24, 2024
erlend-aasland
erlend-aasland reviewed Jan 24, 2024
View reviewed changes
Makefile.pre.in Outdated
@@ -1776,6 +1783,7 @@ PYTHON_HEADERS= \
$(srcdir)/Include/cpython/pthread_stubs.h \
$(srcdir)/Include/cpython/pyatomic.h \
$(srcdir)/Include/cpython/pyatomic_gcc.h \
$(srcdir)/Include/cpython/pyatomic_msc.h \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this Windows only? If so, we can probably discard it here in the *nix build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree I was probably overzealous. I'm happy to have some extra dependencies, false positives being better than false negatives in this case. I'll correct my overzealousness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rhetorical question: Why don't we use GCC, CLANG, or other compilers' ability to generate include dependencies? I'm thinking stuff like gcc -M or clang -M.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rhetorical question: Why don't we use GCC, CLANG, or other compilers' ability to generate include dependencies? I'm thinking stuff like gcc -M or clang -M.

I guess just because the current build system has grown into something that is very hard to maintain, so people in general just try to make as few changes as possible, if any at all. Not to mention configure.ac.

Makefile.pre.in Outdated Show resolved Hide resolved
Makefile.pre.in Outdated
@@ -1681,15 +1681,18 @@ PYTHON_HEADERS= \
$(srcdir)/Include/codecs.h \
$(srcdir)/Include/compile.h \
$(srcdir)/Include/complexobject.h \
$(srcdir)/Include/datetime.h \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the datetime C API capsule. Perhaps this dependency should be more targeted.

Makefile.pre.in Outdated Show resolved Hide resolved
Makefile.pre.in Outdated Show resolved Hide resolved
@smontanaro
Copy link
Contributor Author

Made the changes Erlend requested on MacOS. Pushed and will now test on Linux.

colesbury
colesbury reviewed Jan 24, 2024
View reviewed changes
Makefile.pre.in Outdated Show resolved Hide resolved
@smontanaro
Copy link
Contributor Author

Any further inputs on this, or can it move toward acceptance and merging?

@colesbury
Copy link
Contributor

I don't have any more comments. The change looks good to me, although there is a merge conflict that needs to be resolved.

@smontanaro
Copy link
Contributor Author

I don't understand how to fix that merge conflict. I moved the def'n of PYTHON_HEADERS up as you suggested, but it appears you added pycore_gc.h to PYTHON_HEADERS on main.

I've been chastised for merging from main to a PR in the past. Is this a case where it's necessary?

@colesbury
Copy link
Contributor

Sorry about that. It looks like my PR added pycore_gc.h and pycore_object_stack.h. Here's the diff of Makefile.pre.in since the commit you are based on.

In my experience, merging "main" into the PR branch is okay. Sometimes people accidentally merge in the other direction and that can lead to a giant mess.

@erlend-aasland might have better ideas, but my suggestion would be (if using git from the command line):

  1. git checkout main
  2. git pull (update your local main branch)
  3. git checkout issue114505
  4. git merge main (merge main into issue114505)

And then resolve the merge conflict (make sure that pycore_gc.h and pycore_object_stack.h are listed in PYTHON_HEADERS).

Let me know if you'd like help with it. I think I have permissions on GitHub to resolve the merge conflict and push to the branch, if you'd like me to do it.

@erlend-aasland
Copy link
Contributor

Thanks! I'll take a closer look at this, hopefully before the coming weekend is over.

@smontanaro
Copy link
Contributor Author

Sorry about that. It looks like my PR added pycore_gc.h and pycore_object_stack.h. Here's the diff of Makefile.pre.in since the commit you are based on.

Thanks. I followed your instructions and resolved the conflict in Makefile.pre.in. Hopefully I did it right...

@smontanaro
Copy link
Contributor Author

What's the status on this? I thought I'd resolved the conflicts a week ago. Should I start over?

erlend-aasland
erlend-aasland approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll resolve the conflicts before landing.

@erlend-aasland erlend-aasland enabled auto-merge (squash) February 7, 2024 08:33
@erlend-aasland erlend-aasland linked an issue Feb 7, 2024 that may be closed by this pull request
test_embed failures after switching configure options due to missing Makefile dependencies #114505
Closed
@erlend-aasland erlend-aasland merged commit 2afc718 into python:main Feb 7, 2024
30 checks passed
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@smontanaro @fsc-eriker
pythongh-114505: Add missing header file dependencies (python#114513)
2a0f89d 
Also move PYTHON_HEADERS up and make _testembed.o depend on it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colesbury colesbury colesbury left review comments

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@Bonniemarie216 Bonniemarie216 Bonniemarie216 approved these changes

@zware zware Awaiting requested review from zware

Assignees

@erlend-aasland erlend-aasland

Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_embed failures after switching configure options due to missing Makefile dependencies
4 participants
@smontanaro @colesbury @erlend-aasland @Bonniemarie216