Skip to content

build system runs when it may merely link #77665

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

Closed
grimreaper mannequin opened this issue May 13, 2018 · 6 comments
Closed

build system runs when it may merely link #77665

grimreaper mannequin opened this issue May 13, 2018 · 6 comments
Labels
build The build process and cross-build

Comments

@grimreaper
Copy link
Mannequin

grimreaper mannequin commented May 13, 2018

BPO 33484
Nosy @bitdancer, @grimreaper
PRs
  • bpo-33484: Link instead of run certain test programs #6781
  • 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 = None
    created_at = <Date 2018-05-13.18:38:36.064>
    labels = ['build']
    title = 'build system runs when it may merely link'
    updated_at = <Date 2018-05-15.07:44:31.640>
    user = 'https://github.com/grimreaper'

    bugs.python.org fields:

    activity = <Date 2018-05-15.07:44:31.640>
    actor = 'eitan.adler'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Build']
    creation = <Date 2018-05-13.18:38:36.064>
    creator = 'eitan.adler'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33484
    keywords = ['patch']
    message_count = 5.0
    messages = ['316471', '316473', '316474', '316475', '316476']
    nosy_count = 2.0
    nosy_names = ['r.david.murray', 'eitan.adler']
    pr_nums = ['6781']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue33484'
    versions = []

    @grimreaper
    Copy link
    Mannequin Author

    grimreaper mannequin commented May 13, 2018

    The build system attempts to run certain test code when it can actually just link the code. This is a minor performance optimization but is really a semantic correctness issue.

    @grimreaper grimreaper mannequin added the build The build process and cross-build label May 13, 2018
    @bitdancer
    Copy link
    Member

    Thanks for wanting to improve Python.

    Your PR seems to contain quite a few changes that aren't obviously related to your topic.

    As for the topic, I know there are at least some cases where we really do have to *run* the code to prove the function is supported by the platform. That is, there are platforms that define the function but it doesn't actually work. I don't know if that applies to the things you are changing, and it will require input from people with more knowledge of the build system than I to confirm that your changes are a good idea.

    @grimreaper
    Copy link
    Mannequin Author

    grimreaper mannequin commented May 13, 2018

    The unrelated changes are likely due to the "regen" step. I am using newer tools than previously used. It is unclear to me why we include generated files in the repository. Some projects prefer this to avoid the need to have autotools installed on the build machine.

    With regard to "some cases where we really do have to *run* the code to prove the function" you will notice that this is not a complete search and replace: I manually looked through the various uses of AC_RUN_IFELSE and only changed the ones that I thought should be changed. If someone has more knowledge of more arcane platforms and disagrees with my assessment, I will happily revert those changes.

    @bitdancer
    Copy link
    Member

    Yes, we are one of those projects that wants it to be possible to build python without having the tools that generate build artifacts. You will note that we also check in build artifacts that are produced by our own python scripts, to avoid the problem of trying to build python when you don't yet have a python.

    Please use the same version of autotools as were used to generate the current files. Upgrading autotools is done in a separate PR that only does the upgrade (and I don't know how we decide when to do that...)

    Actually, you could just generate the PR with the changes to the source files, not the build artifacte, for the initial review.

    @grimreaper
    Copy link
    Mannequin Author

    grimreaper mannequin commented May 13, 2018

    I believe I have removed the regen step from all my patches. I will separately upgrade the generated files (particularly since it is only a minor version bump).

    @grimreaper grimreaper mannequin changed the title build system runs when it merely link build system runs when it may merely link May 15, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @encukou
    Copy link
    Member

    encukou commented Mar 19, 2024

    This is a minor performance optimization but is really a semantic correctness issue.

    Avoiding 4 processes (on runs without cache) is a very minor optimization indeed. And I don't see the semantic correctness -- surely we want the functions to not return an error indicator when called.
    I don't think the change is worth the possibility of breaking on the mentioned arcane platforms.
    I to close this PR and issue; please comment if you disagree.

    @encukou encukou closed this as completed Mar 19, 2024
    @encukou encukou closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2024
    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
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants