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

python3.pkgs.buildPython*: allow overriding of the stdenv #173411

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented May 17, 2022

With this change it is possible to pass in stdenv directly to
buildPython* or override it using e.g.

numpy.overridePythonAttrs(_: {
  stdenv = clangStdenv;
})
Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@FRidh
Copy link
Member Author

FRidh commented May 17, 2022

I wish the compiler would go into nativeBuildInputs instead, but we're not there. Now, we can at least offer the following possibility to override the compiler.

@FRidh
Copy link
Member Author

FRidh commented May 17, 2022

I wonder how well this works with sysconfigdata.

@FRidh
Copy link
Member Author

FRidh commented Jun 1, 2022

Actually it is maybe better to pass in mkDerivation instead as that gives more flexibility.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 8, 2023
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 27, 2023
@SomeoneSerge
Copy link
Contributor

I expect this change would allow for a cleaner solution to #217913 #217878 #218265, where CUDA dictates the choice of a toolchain for building downstream (python) packages

@FRidh
Copy link
Member Author

FRidh commented Feb 27, 2023

Try it! If it seems to work we can merge this. If this solution turns out not to be ideal we can always follow-up with another solution.

@SomeoneSerge
Copy link
Contributor

Try it! If it seems to work we can merge this

I did test this while working on #218265
It worked.
@FRidh expressed concerns about what implications this change may have for cross-compilation

  • Test cross

@ConnorBaker
Copy link
Contributor

Following up on this, @SomeoneSerge @FRidh any ideas on whether this would break (or otherwise worsen) cross-compilation, or whether that's easily tested?

Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

I'm not entirely sure what "breaking" cross would mean in this context.

I rebased this PR on 12bdeb0 and then ran some cross builds of the Coverage Python package, which includes a C extension (if that's the correct terminology). (Numpy was too big for some quick tests. It requires a cross GFortran, which is not cached.) This is on an x86_64-linux.

Cross without overriding stdenv

pkgsCross.aarch64-multiplatform.python3Packages.coverage:

$ file -b /nix/store/k5cysvrkgfcpif713a6xa2rx1d5pwyi1-python3.10-coverage-7.2.1-aarch64-unknown-linux-gnu/lib/python3.10/site-packages/coverage/tracer.cpython-310-aarch64-linux-gnu.so 
ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, not stripped

Cross overriding stdenv to use Clang

pkgsCross.aarch64-multiplatform.python3Packages.coverage.overridePythonAttrs (_: { stdenv = pkgsCross.aarch64-multiplatform.clangStdenv; }):

$ file -b /nix/store/4n6rzmdsps2587sip62wnfjchaaxw15g-python3.10-coverage-7.2.1-aarch64-unknown-linux-gnu/lib/python3.10/site-packages/coverage/tracer.cpython-310-aarch64-linux-gnu.so
ELF 64-bit LSB shared object, ARM aarch64, version 1 (SYSV), dynamically linked, not stripped

Cross overriding with a non-cross stdenv

This is the only scenario in which you could claim cross is "broken."
By explicitly passing a non-cross stdenv, which is then indiscriminately used to compile C bits, the resulting objects are indeed non-cross-compiled.
I do not agree this should be considered broken, but, it might catch someone by surprise at first.

pkgsCross.aarch64-multiplatform.python3Packages.coverage.overridePythonAttrs (_: { stdenv = clangStdenv; }):

$ file -b /nix/store/m2pm702mnmjakhqhdp60cn0p49p43rdl-python3.10-coverage-7.2.1/lib/python3.10/site-packages/coverage/tracer.cpython-310-aarch64-linux-gnu.so 
ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, not stripped

P.S.: Fairly eager to get this merged because it allows us to fix pybind11 when building with Clang 16. I'm not sure how often master is merged to staging during a release window so might this require targeting at staging instead?

@FRidh
Copy link
Member Author

FRidh commented Oct 18, 2023

Cross overriding stdenv to use Clang

Is indeed what I wanted to have verified. Thanks for checking.

@toonn toonn marked this pull request as draft October 19, 2023 16:42
@toonn toonn changed the base branch from master to staging October 19, 2023 16:48
With this change it is possible to pass in `stdenv` directly to
`buildPython*` or override it using e.g.

```
numpy.overridePythonAttrs(_: {
  stdenv = clangStdenv;
})
```
@toonn toonn marked this pull request as ready for review October 19, 2023 17:10
@toonn toonn merged commit 8f18239 into NixOS:staging Oct 19, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants