gh-119538: Add missing build dependencies #125998
Conversation
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Thanks for the PR. I'm a little puzzled why you closed your previous PR and opened a new (and pretty much identical) PR without addressing my previous review remarks. |
I made the mistake of clicking discard commit when updating my repository, which caused the previous PR to be automatically closed. I'm sorry about this. Also, I've updated the code based on your previous comments. |
|
|
||
| Modules/posixmodule.o: $(srcdir)/Modules/posixmodule.c $(srcdir)/Modules/posixmodule.h | ||
| Modules/posixmodule.o: $(srcdir)/Modules/posixmodule.c $(srcdir)/Modules/posixmodule.h $(srcdir)/Include/internal/pycore_emscripten_trampoline.h |
There was a problem hiding this comment.
Why all these pycore_emscripten_trampoline.h dependencies? Can you elaborate?
There was a problem hiding this comment.
I traced the build of Modules/posixmodule.o and found that Make calls pycore_emscripten_trampoline.h, which I can of course remove.
There was a problem hiding this comment.
Please remove all the pycore_emscripten_trampoline.h deps; they are only relevant for that particular build config, and we need a better way to do those anyway.
Makefile.pre.in
Outdated
| @@ -1293,6 +1297,7 @@ PYTHON_HEADERS= \ | |||
| $(srcdir)/Include/internal/pycore_uop_metadata.h \ | |||
| $(srcdir)/Include/internal/pycore_warnings.h \ | |||
| $(srcdir)/Include/internal/pycore_weakref.h \ | |||
| $(srcdir)/Python/condvar.h \ | |||
There was a problem hiding this comment.
As I mentioned in my previous review, this should be a dependency of ceval_gil.o (and not ceval.o, as it currently is). IOW:
- I don't think it is right to include it in
PYTHON_HEADERS - I think we should remove
condvar.hfrom theceval.otarget - I think we need a separate target for
ceval_gil.o
There was a problem hiding this comment.
If we declare ceval_gil.o as a separate target, what line should it go on?
There was a problem hiding this comment.
It should be near the ceval.o target.
There was a problem hiding this comment.
I already updated the PR. Could you add a label “skip news”?
Makefile.pre.in
Outdated
| $(srcdir)/Objects/mimalloc/stats.c \ | ||
| $(srcdir)/Objects/mimalloc/page-queue.c \ |
There was a problem hiding this comment.
Please make sure you add lines so the list is still alphabetically sorted. Moreover, page-queue.c is already a dependency of the page.o subtarget; I think we should remove that target, as page.c is #included in the source, not compiled as a separate source file.
Makefile.pre.in
Outdated
| @@ -346,7 +346,8 @@ MODULE_OBJS= \ | |||
| Modules/main.o \ | |||
| Modules/gcmodule.o | |||
|
|
|||
| IO_H= Modules/_io/_iomodule.h | |||
| IO_H= Modules/_io/_iomodule.h \ | |||
There was a problem hiding this comment.
Please add a line break first, similar to how it's done with IO_OBJS below.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
FYI, @Meiye-lj, we discourage force-pushes, as those does not play well with the review and CI experience. Instead of rebasing, please use |
Thank you, I noticed. |
sync with main
This PR fixes an issue in the Makefile.pre.in of Cpython. Specifically, previously, any modifications of files like Objects/mimalloc/alloc-override.c would not trigger a rebuild of Objects/obmalloc.o. The PR fixes this by including them as additional dependencies. This addresses #120096 and #119538. #119538 and #119647 resolve some missing dependency errors, this PR further fixes missing dependency errors.
Issue: #119538