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

Remove incorrect uses of recursive make #66555

Closed
Sjlver mannequin opened this issue Sep 8, 2014 · 29 comments
Closed

Remove incorrect uses of recursive make #66555

Sjlver mannequin opened this issue Sep 8, 2014 · 29 comments
Labels
build The build process and cross-build type-bug An unexpected behavior, bug, or error

Comments

@Sjlver
Copy link
Mannequin

Sjlver mannequin commented Sep 8, 2014

BPO 22359
Nosy @loewis, @brettcannon, @doko42, @pitrou, @xdegaye, @vadmium, @koobs
Files
  • makefile_parallel.patch: Patch: Remove incorrect uses of recursive make
  • koobs-freebsd9.python-2.7-build.912.txt
  • cross-chgeset-c2a53aa27cad.patch
  • separate-regen.patch: Separate "make graminit" etc targets
  • cross-chgeset-c2a53aa27cad_2.patch
  • crossbuild-sources-readonly.patch: Do not modify build source files
  • crossbuild-sources-readonly_2.patch: _freeze_importlib and pgen are cross-built
  • 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:

    assignee = None
    closed_at = <Date 2016-04-24.03:41:09.132>
    created_at = <Date 2014-09-08.08:44:29.657>
    labels = ['type-bug', 'build']
    title = 'Remove incorrect uses of recursive make'
    updated_at = <Date 2016-04-24.03:41:09.131>
    user = 'https://bugs.python.org/Sjlver'

    bugs.python.org fields:

    activity = <Date 2016-04-24.03:41:09.131>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-04-24.03:41:09.132>
    closer = 'martin.panter'
    components = ['Build']
    creation = <Date 2014-09-08.08:44:29.657>
    creator = 'Sjlver'
    dependencies = []
    files = ['36570', '40063', '42164', '42169', '42517', '42530', '42554']
    hgrepos = []
    issue_num = 22359
    keywords = ['patch']
    message_count = 29.0
    messages = ['226563', '226797', '227186', '227187', '230766', '238199', '245110', '245112', '245117', '245119', '245122', '245365', '245366', '247650', '247651', '261775', '261786', '261799', '261824', '263721', '263793', '263815', '263875', '263891', '263898', '263902', '263915', '264035', '264089']
    nosy_count = 11.0
    nosy_names = ['loewis', 'brett.cannon', 'doko', 'pitrou', 'Arfrever', 'xdegaye', 'python-dev', 'martin.panter', 'koobs', 'Sjlver', 'WanderingLogic']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22359'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @Sjlver
    Copy link
    Mannequin Author

    Sjlver mannequin commented Sep 8, 2014

    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.

    @Sjlver Sjlver mannequin added build The build process and cross-build labels Sep 8, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Sep 11, 2014

    Martin, Matthias, do you think this would break any legitimate use?

    @pitrou pitrou added type-bug An unexpected behavior, bug, or error and removed build The build process and cross-build labels Sep 11, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 20, 2014

    New changeset c2a53aa27cad by Antoine Pitrou in branch 'default':
    Issue bpo-22359: Remove incorrect uses of recursive make. Patch by Jonas Wagner.
    https://hg.python.org/cpython/rev/c2a53aa27cad

    @pitrou
    Copy link
    Member

    pitrou commented Sep 20, 2014

    Ok, I've pushed the patch. Let's see if anyone complains.

    @pitrou pitrou closed this as completed Sep 20, 2014
    @WanderingLogic
    Copy link
    Mannequin

    WanderingLogic mannequin commented Nov 6, 2014

    Sorry, I'm complaining. Cross builds broke. Please see bpo-22809.

    @doko42
    Copy link
    Member

    doko42 commented Mar 16, 2015

    reopening, breaks cross builds. I'll have a look

    @doko42 doko42 reopened this Mar 16, 2015
    @koobs koobs added the build The build process and cross-build label Jun 4, 2015
    @koobs
    Copy link

    koobs commented Jun 10, 2015

    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
    *** [Parser/pgen] Error code 1

    pgenmain.c:(.text+0x258): undefined reference to `_Py_pgen'
    *** [Parser/pgen] Error code 1
    1 error
    *** [Include/graminit.h] Error code 2

    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?

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2015

    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 $(PGEN) for the host, and then rely on it not being rebuilt. Maybe there is a better way, like changing the $(PGEN) command line to use a $(HOST_CC) or something. (I’m not familiar with the build system; certainly not when cross compiling.) But you could also try and identify the dependency that is causing $(PGEN) to be rebuilt, for example with Gnu Make:

    $ 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'.

    @koobs
    Copy link

    koobs commented Jun 10, 2015

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Jun 10, 2015

    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.

    @koobs
    Copy link

    koobs commented Jun 15, 2015

    Initial commit to default is done. Backport to 2.7 & 3.4 branches remains to do

    @koobs koobs added easy and removed build The build process and cross-build labels Jun 15, 2015
    @koobs
    Copy link

    koobs commented Jun 15, 2015

    Add 3.5 to list of versions merging required for

    @koobs
    Copy link

    koobs commented Jul 30, 2015

    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)

    @koobs koobs added the stale Stale PR or inactive for long period of time. label Jul 30, 2015
    @vadmium
    Copy link
    Member

    vadmium commented Jul 30, 2015

    Since finishing committing this to other branches seems to be controversial, it might help resolving the cross compilation problem first.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Mar 14, 2016

    This patch improves changeset c2a53aa27cad by allowing cross-compilation:

    The following checks have been made:

    • successfull cross-compilation of python3.4 after retrofiting changeset
      c2a53aa27cad to python3.4 since I don't have a working cross-compilation
      setup for python3.6 at the moment
    • successfull native compilation of python3.6 (default branch)
    • Python/graminit.c is re-generated after touching Parser/pgenmain.c on a
      native build

    @vadmium
    Copy link
    Member

    vadmium commented Mar 14, 2016

    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:

    • Reliable way to update generated files when necessary
    • Shared prerequisites unsafe with recursive concurrent $(MAKE)
    • Reasons to avoid regenerating files (not out of date, cross compilation, less build steps and prerequisites; see python-dev)
    • Building with read-only sources (see revision 67ed8a6905c3, then r87558)

    @vadmium vadmium removed the easy label Mar 14, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Mar 15, 2016

    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.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Mar 15, 2016

    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.

    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:

    cross_compiling=$$(echo $(PYTHON_FOR_BUILD) | sed "s/.*_PYTHON_HOST_PLATFORM.*/yes/"); \
    if [ "$$cross_compiling" = "yes" ]; then \
          touch $@;
    else \
          # The original recipes.
    fi
    

    So the built binaries $(PGEN) and Programs/_freeze_importlib would be dummy empty files, but that would not prevent Programs/_freeze_importlib.o, $(LIBRARY_OBJS_OMIT_FROZEN) and $(PGENOBJS) to be needlessly cross-compiled.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Apr 19, 2016

    Here is a patch that does not use GNU make conditionals.
    Patch tested by running the test suite both natively and after a
    cross-compilation.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 20, 2016

    I think this may be a viable solution Xavier. See review suggestion about building from read-only source though.

    @vadmium vadmium removed the stale Stale PR or inactive for long period of time. label Apr 20, 2016
    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Apr 20, 2016

    Martin, thanks for the review and the suggestions.

    The attached patch makes the following changes:

    • do not modify the source files so as not to break builds made from
      read-only build sources (as per bpo-15819)
    • the cross-build copies the graminit.[ch] files to the build directory
    • fix tab indentation

    Patch tested by checking that the graminit.[ch] time stamps do not
    change in the source directory afer a build and after a cross-build,
    and by running the test suite both natively and on android emulator.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 21, 2016

    Looks pretty good. One more question: Why do you make the linking of _freeze_importlib conditional, but always build $(PGEN)?

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Apr 21, 2016

    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 ?

    @vadmium
    Copy link
    Member

    vadmium commented Apr 21, 2016

    I see. I guess it would keep the makefile simpler if we cross-compiled both programs, and never used them.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 21, 2016

    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).

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Apr 21, 2016

    _freeze_importlib and pgen are cross-built in this patch. Patch tested with a run of the testsuite after a cross-build.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 23, 2016

    New changeset 66e40df31fac by Martin Panter in branch '3.5':
    Issue bpo-22359: Disable running cross-compiled _freeze_importlib and pgen
    https://hg.python.org/cpython/rev/66e40df31fac

    New changeset c36b9ef8ff39 by Martin Panter in branch 'default':
    Issue bpo-22359: Merge cross-compilation fix from 3.5
    https://hg.python.org/cpython/rev/c36b9ef8ff39

    New changeset 0f7a299c6d50 by Martin Panter in branch '2.7':
    Issue bpo-22359: Avoid recursive $(MAKE); disable running cross-compiled pgen
    https://hg.python.org/cpython/rev/0f7a299c6d50

    @vadmium
    Copy link
    Member

    vadmium commented Apr 24, 2016

    Thanks for your help with this Xavier.

    @vadmium vadmium closed this as completed Apr 24, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    build The build process and cross-build type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants