gh-114505: Add missing header file dependencies #114513
Conversation
…but these were called out for me)
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 \ | |||
There was a problem hiding this comment.
Isn't this Windows only? If so, we can probably discard it here in the *nix build.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Rhetorical question: Why don't we use GCC, CLANG, or other compilers' ability to generate include dependencies? I'm thinking stuff like
gcc -Morclang -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
| @@ -1681,15 +1681,18 @@ PYTHON_HEADERS= \ | |||
| $(srcdir)/Include/codecs.h \ | |||
| $(srcdir)/Include/compile.h \ | |||
| $(srcdir)/Include/complexobject.h \ | |||
| $(srcdir)/Include/datetime.h \ | |||
There was a problem hiding this comment.
This is for the datetime C API capsule. Perhaps this dependency should be more targeted.
|
Made the changes Erlend requested on MacOS. Pushed and will now test on Linux. |
|
Any further inputs on this, or can it move toward acceptance and merging? |
|
I don't have any more comments. The change looks good to me, although there is a merge conflict that needs to be resolved. |
|
I don't understand how to fix that merge conflict. I moved the def'n of I've been chastised for merging from |
|
Sorry about that. It looks like my PR added 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):
And then resolve the merge conflict (make sure that 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. |
|
Thanks! I'll take a closer look at this, hopefully before the coming weekend is over. |
Thanks. I followed your instructions and resolved the conflict in |
|
What's the status on this? I thought I'd resolved the conflicts a week ago. Should I start over? |
Also move PYTHON_HEADERS up and make _testembed.o depend on it.
@colesbury and @erlend-aasland pointed out problems with some header file dependencies in
Makefile.pre.in. This PR corrects the obvious problems, the definition ofPYTHON_HEADERSand the dependencies forPrograms/_testembed.o.I believe the various
*_HEADERSdefinitions are now correct (I found nothing else amiss besidesPYTHON_HEADERS). I've not yet looked at any other.o -> .hdependencies, but will do that.