-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-131556: Fix build-details.json
Makefile target
#131558
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
Conversation
Use the contents of `pybuilddir.txt` as a prefix for the build-details.json path, if it exists.
Things I've tested locally:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, it's very interesting approach that the generated Makefile could be different following to pybuilddir.txt :)
@erlend-aasland Do you have better ideas?
The |
Ah thanks you are right, but still good to me |
@@ -936,8 +937,8 @@ pybuilddir.txt: $(PYTHON_FOR_BUILD_DEPS) | |||
exit 1 ; \ | |||
fi | |||
|
|||
build-details.json: pybuilddir.txt | |||
$(RUNSHARED) $(PYTHON_FOR_BUILD) $(srcdir)/Tools/build/generate-build-details.py `cat pybuilddir.txt`/build-details.json | |||
$(shell [ -f pybuilddir.txt ] && cat pybuilddir.txt)/build-details.json: pybuilddir.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot use $(shell
and should probably forget it exists :)
It is a GNU-ism and will break on any other Make implementation, such as the default Make on BSD.
If cpython were to move to require GNU Make then there are many changes which could be made to this Makefile that would make it significantly smaller, but alas, compatibility...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colesbury, sorry for the delay getting to this.
After exploring our options a bit, I believe we should move the pybuilddir.txt
value calculation from python -m sysconfig --generate-posix-vars
to Makefile.pre.in
.
This issue is only a symptom of the bigger issue of us not being able to write Makefile targets for files under PYBUILDDIR
, for which we already adopted similar sub-optimal workarounds in other places. This detail of the build process design is mostly historical, and I don't think there should be any reason why we couldn't define a PYBUILDDIR
variable to Makefile.pre.in
— we should have access to the required information there.
I am gonna open a PR implementing this and see how the buildbots react 😅
Thanks for the explanations |
Yup. As long as the information is known and frozen at the time of running Makefiles have quite a bit of flexibility for the commands you run during the build, but I unfortunately very little (portable) flexibility in terms of defining the actual graph of the build jobs e.g. target filenames. That is, in very large part, why configure scripts were invented. I suspect there may not be any use case for pybuilddir.txt to change without a reconfigure. It can be useful for visibility by commands being run (such as the python exe) without imparting meaning to Make itself. Using a Makefile variable |
Yeah, it is. It's just more annoying to manipulate data there IMO, and depending on exactly what we are doing with the data, it may introduce the need of duplicating it in the Windows build-system. Though, in this case, that isn't a big worry, and even if it was more of a concern, the benefits would still be far worth it.
There is a tiny bit of wiggle room by using the
No, indeed, |
Use the contents of
pybuilddir.txt
as a prefix for the build-details.json path, if it exists.build-details.json
#131556