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
Remove incorrect uses of recursive make #66555
Comments
The attached patch fixes issues with Python's Makefile, which manifest when doing parallel builds. The Makefile invoked "make" recursively for some targets. This caused some files (which were depended upon by multiple targets) to be built by both the original "make" and the sub-"make". Besides duplicate work, this caused failed builds with non-threadsafe compilers. The proposed patch removes recursive calls to "make", and instead builds all targets in the same "make" process. |
Martin, Matthias, do you think this would break any legitimate use? |
New changeset c2a53aa27cad by Antoine Pitrou in branch 'default': |
Ok, I've pushed the patch. Let's see if anyone complains. |
Sorry, I'm complaining. Cross builds broke. Please see bpo-22809. |
reopening, breaks cross builds. I'll have a look |
We've (FreeBSD) seen and received several random and intermittent reports of failures during this stage of the build for all versions of Python, resulting in errors such as, among others: Parser/pgen.o: file not recognized: File truncated pgenmain.c:(.text+0x258): undefined reference to `_Py_pgen' See: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=200622 Given the existing patch breaks cross-builds, is there an alternative or safer way to make this section of the build more robust in order to make progress? |
FWIW two times recently I saw strange intermittent build errors. I use “make -j2” on Linux. The one time that I investigated was a Python module (math or cmath perhaps?) failing to build because some dependency was zero bytes, and looking through the build log, I saw duplicate compiler commands lines printed out one after the other to build that dependency. |
koobs: It sounds like applying the fix for this bug to the Python 2 branch might satisfy you, right? It only seems to have been applied to 3.5+ so far. Matt: It sounds like you are trying to pre-compile $ make --debug=b Parser/pgen
Updating goal targets....
Prerequisite `Include/Python.h' is newer than target `Parser/acceler.o'.
Must remake target `Parser/acceler.o'.
[. . .]
Prerequisite `Parser/acceler.o' is newer than target `Parser/pgen'.
[. . .]
Must remake target `Parser/pgen'. |
martin: yes, if the incorrect usage of recursive make is in the 2.7 and 3.4 branches and the fix is relevant to the issues I reported. Of course all subsequent changes (fixing cross-builds) should be applied there too. |
My math and cmath issue seems to be completely separate. I opened bpo-24421 for that, so sorry for the noise here :) I think the patch here should be applied (or adapted if necessary) to the 3.4 and 2.7 branches. If it breaks cross compilation, then the cross compilation technique needs improving instead; perhaps see bpo-22625 for that. |
Initial commit to default is done. Backport to 2.7 & 3.4 branches remains to do |
Add 3.5 to list of versions merging required for |
One of the FreeBSD buildbots (koobs-freebsd9) just failed on a 2.7 build due to this issue. Full log attached, as the bug is intermittent and non-deterministic (though appears more likely during high build host loads). This issue no longer 'needs patch' as it has was committed to default (3.5) This really needs to be backported to the rest of supported bugfix branches as soon as possible (along with the cross-build regression outlined in bpo-22809 (a duplicate of 22625) |
Since finishing committing this to other branches seems to be controversial, it might help resolving the cross compilation problem first. |
This patch improves changeset c2a53aa27cad by allowing cross-compilation: The following checks have been made:
|
Thanks for you work Xavier. Making it conditional on cross compilation is another approach I hadn’t thought of. But I am a bit worried at the new makefile syntax [ifeq directive and $(findstring) function]. I suspect it is Gnu specific, and that Python tries to support other versions of Make as well. Koobs: does Free BSD accept this makefile syntax? Much as I hate the configure.ac stuff, I guess it could be used to enable and disable conditional stuff in the makefile, so that might be another option. Whatever the end solution is, I think we need a big comment in the makefile pointing out the quirks and different people’s interests:
|
Here is a patch implementing my idea to make file reneration a separate step, not automatically run during the normal build process. But it is sounding like other people don’t like this idea, so Xavier’s approach might be less controversial even if it is more complicated. |
Yes, they are both GNU extensions :( To avoid modifying configure.ac since the information is already available, the recipes could be modified with the following conditional:
So the built binaries $(PGEN) and Programs/_freeze_importlib would be dummy empty files, but that would not prevent Programs/_freeze_importlib.o, |
Here is a patch that does not use GNU make conditionals. |
I think this may be a viable solution Xavier. See review suggestion about building from read-only source though. |
Martin, thanks for the review and the suggestions. The attached patch makes the following changes:
Patch tested by checking that the graminit.[ch] time stamps do not |
Looks pretty good. One more question: Why do you make the linking of _freeze_importlib conditional, but always build $(PGEN)? |
Yes, this is not consistent. The cross-build is correct when both are linked or when both are not linked. And $(PGENOBJS) are cross-compiled in both cases. Should both programs not be created ? |
I see. I guess it would keep the makefile simpler if we cross-compiled both programs, and never used them. |
For the record, to fix this in 2.7 will involve backporting revision c2a53aa27cad, plus Xavier’s patch (except the freeze_importlib parts which is not relevant to 2.7). |
_freeze_importlib and pgen are cross-built in this patch. Patch tested with a run of the testsuite after a cross-build. |
New changeset 66e40df31fac by Martin Panter in branch '3.5': New changeset c36b9ef8ff39 by Martin Panter in branch 'default': New changeset 0f7a299c6d50 by Martin Panter in branch '2.7': |
Thanks for your help with this Xavier. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: