Skip to content

pythonCatchConflictsHook: scan $out, not sys.path (2)#287957

Merged
Lassulus merged 4 commits intoNixOS:stagingfrom
DavHau:python
Feb 13, 2024
Merged

pythonCatchConflictsHook: scan $out, not sys.path (2)#287957
Lassulus merged 4 commits intoNixOS:stagingfrom
DavHau:python

Conversation

@DavHau
Copy link
Member

@DavHau DavHau commented Feb 11, 2024

Based on #284067
Fixes #283695

  • pythonCatchConflictsHook: scan only $out for conflicts instead of $PYTHONPATH (see Towards a conflict free python build-system #283695 on why this is better)
  • improve error message: print part of the dependency tree that leads to the conflict
  • add test cases for the hook testing 3 important scenarios
    • ensure hook ignores build time packages
    • ensure hook catches simple conflict
    • ensure hook catches transitive conflict

cc @mweinelt @phaer @FRidh

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Feb 11, 2024
@DavHau DavHau force-pushed the python branch 2 times, most recently from ed5a679 to 7d978f2 Compare February 11, 2024 07:55
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Feb 11, 2024
@phaer
Copy link
Member

phaer commented Feb 11, 2024

Thanks! Hope to take a closer look this week.

One thing we should most probably do before a merge is to handle catch_conflicts_py2.py in some way

@DavHau DavHau requested a review from mweinelt as a code owner February 12, 2024 12:32
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Feb 12, 2024
@DavHau
Copy link
Member Author

DavHau commented Feb 12, 2024

One thing we should most probably do before a merge is to handle catch_conflicts_py2.py in some way

I now made the hook compatible to all python 3 version we still support (from 3.8). Also tested that it works on pypy.

After this, only python2.7 still uses the legacy hook, which seems fine, as it's only used for a handful of packages anymore. I feel mentioning the difference between python 2.7 and 3.x in the manual might confuse people more than it actually helps, but happy to change my mind on this.

This changes the non-legacy version of pythonCatchConflictsHook
to recursively scan the output of the target derivation  as well
as its propagatedBuildInputs for duplicate dependencies.

Previously, we did scan sys.path but did prove problematic as it
produced false positives i.e. when build-time dependencies of
hooks - such as setuptools in pythonCatchConflictsHook itself -
where mistakenly flagged as duplicates; even though the are
not included in the outputs of the target dervation.

As all python runtime-dependencies are currently passed via
propagatedBuildInputs in nixpkgs, scanning that plus
site-packages seems sufficient to catch all conflicts that
matter at runtime and less likely to produce false positives.

The legacyHook in catch_conflicts_py2.py needs to be migrated
as well, if it's still needed.
@DavHau DavHau force-pushed the python branch 2 times, most recently from 8b0d495 to eb39b6a Compare February 12, 2024 12:53
Copy link
Member

@phaer phaer left a comment

Choose a reason for hiding this comment

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

Just to recap for readers: Python 2.7 would still scan sys.path instead of the actual outputs. As the motivation of this PR are false-positives which mostly occur if you use pyproject.toml without setuptools, this seems ok for me - but happy to hear more opinions or even suggestions to for documentation :)

Code LGTM!

@DavHau DavHau requested a review from phaer February 12, 2024 13:25
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 12, 2024
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 13, 2024
@Lassulus Lassulus merged commit 38905fc into NixOS:staging Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: clean-up This PR removes packages or removes other cruft 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants