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.

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

gh-114883: Fix Makefile dependency tree for non-jit builds #114884

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

zware
Copy link
Member

@zware zware commented Feb 1, 2024

@erlend-aasland

This comment was marked as resolved.

@brandtbucher
Copy link
Member

brandtbucher commented Feb 2, 2024

Thanks! I was working on this at the same time as you, and ended up with this:

diff --git a/Makefile.pre.in b/Makefile.pre.in
index fff3d3c491..fc28d7f919 100644
--- a/Makefile.pre.in
+++ b/Makefile.pre.in
@@ -2643,7 +2643,10 @@ config.status:   $(srcdir)/configure
 Python/asm_trampoline.o: $(srcdir)/Python/asm_trampoline.S
        $(CC) -c $(PY_CORE_CFLAGS) -o $@ $<
 
-Python/jit.o: regen-jit
+Python/jit.o: jit_stencils.h
+
+jit_stencils.h: $(shell find $(srcdir)/Tools/jit/*) $(srcdir)/Python/executor_cases.c.h pyconfig.h
+       @REGEN_JIT_COMMAND@
 
 .PHONY: regen-jit
 regen-jit:

This seems to work correctly everywhere. It also seems a bit simpler (and it has a dependency on Tools/jit that's recursively deep, since I'm thinking of adding some subdirectories in the future).

For my own learning, do you mind explaining why the changes in configure are necessary, as well as the compile command for jit.o (or other differences I missed)? I don't think the location of jit_stencils.h needs to be configurable.

@zware
Copy link
Member Author

zware commented Feb 2, 2024

Thanks! I was working on this at the same time as you, and ended up with this:

diff --git a/Makefile.pre.in b/Makefile.pre.in
index fff3d3c491..fc28d7f919 100644
--- a/Makefile.pre.in
+++ b/Makefile.pre.in
@@ -2643,7 +2643,10 @@ config.status:   $(srcdir)/configure
 Python/asm_trampoline.o: $(srcdir)/Python/asm_trampoline.S
        $(CC) -c $(PY_CORE_CFLAGS) -o $@ $<
 
-Python/jit.o: regen-jit
+Python/jit.o: jit_stencils.h
+
+jit_stencils.h: $(shell find $(srcdir)/Tools/jit/*) $(srcdir)/Python/executor_cases.c.h pyconfig.h
+       @REGEN_JIT_COMMAND@
 
 .PHONY: regen-jit
 regen-jit:

That works, but is pretty platform dependent: keeping shell and find both happy everywhere sounds like a headache we should avoid if we can :)

This seems to work correctly everywhere. It also seems a bit simpler (and it has a dependency on Tools/jit that's recursively deep, since I'm thinking of adding some subdirectories in the future).

Ideally, jit_stencils.h should indeed depend on everything that can possibly contribute to it. In practice, it ought to be ok to just depend on the most obvious files that are most likely to change and rely on regen-jit for the rest.

For my own learning, do you mind explaining why the changes in configure are necessary,

That's to make the dependency on jit_stencils.h dependent on --enable-experimental-jit: @JIT_STENCILS_H@ is replaced with an empty string for normal builds, or the filename for JIT builds. Basically it's just for pruning the dependency tree.

as well as the compile command for jit.o (or other differences I missed)?

The compile command is basically just to avoid relying on an implicit recipe. I'm not certain that I got it right; I just copied it from a nearby target, but it seems to work as expected.

I don't think the location of jit_stencils.h needs to be configurable.

Location doesn't, but whether it needs to be created does :)

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Thanks for the explainer!

Makefile.pre.in Outdated
Comment on lines 2647 to 2651
JIT_DEPS = \
$(srcdir)/Python/jit.c \
$(srcdir)/Python/executor_cases.c.h \
pyconfig.h \
$(srcdir)/Tools/jit/template.c \
Copy link
Member

@brandtbucher brandtbucher Feb 3, 2024

Choose a reason for hiding this comment

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

jit_stencils.h doesn't depend on jit.c (it's the other way around).

If there's no way to express a dependency on everything inside of Tools/jit (recursive), maybe we can just approximate it two levels deep with something like this?

Suggested change
JIT_DEPS = \
$(srcdir)/Python/jit.c \
$(srcdir)/Python/executor_cases.c.h \
pyconfig.h \
$(srcdir)/Tools/jit/template.c \
JIT_DEPS = \
$(srcdir)/Tools/jit/*/* \
$(srcdir)/Tools/jit/* \
$(srcdir)/Python/executor_cases.c.h \
pyconfig.h

Copy link
Member Author

Choose a reason for hiding this comment

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

GNU Make 4.3 (from Ubuntu 22.04) doesn't like $(srcdir)/Tools/jit/*/* because there are no directories in Tools/jit right now, I'm not certain that you can use multiple globs in a Makefile path name, and that's a bit more liberal than I think we ought to be here anyway.

Point taken about jit.c, though; removed it from JIT_DEPS, added $(srcdir)/Tools/jit/*.py, and switched from template.c to *.c. If and when things get rearranged into a hierarchy, the rules should be fairly straightforward to update.

@zware zware enabled auto-merge (squash) February 3, 2024 22:55
@zware zware merged commit 1032326 into python:main Feb 3, 2024
32 checks passed
@zware zware deleted the make_jit_fix branch February 3, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants