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

stdenvAdapters.useLibsFrom: use targetStdenv.cc.override #281371

Conversation

ConnorBaker
Copy link
Contributor

@ConnorBaker ConnorBaker commented Jan 16, 2024

Description of changes

stdenvAdapters.useLibsFrom now uses targetStdenv.cc.override, assuming that the cc attribute of the provided environment is a wrapped compiler. Beyond the benefits of avoiding a roundtrip wrapping, under cross-compilation we now use the copy of coreutils available to targetStdenv.

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.

CC @NixOS/cuda-maintainers


Add a 👍 reaction to pull requests you find important.

@ConnorBaker ConnorBaker added the 6.topic: cuda Parallel computing platform and API label Jan 16, 2024
@ConnorBaker ConnorBaker self-assigned this Jan 16, 2024
@ConnorBaker
Copy link
Contributor Author

ConnorBaker commented Jan 16, 2024

Important

This report was run against 01f2ee08fe9490da736a886e40f06fe75c223fa1.

Note

Template nixpkgs-review command:

PR=281371; \
SYSTEM="aarch64-linux"; \
CUDA_SUPPORT="true"; \
CUDA_CAPABILITIES='[ "7.5" ]'; \
nixpkgs-review pr "$PR" \
  --system "$SYSTEM" \
  --no-shell \
  --checkout commit \
  --allow aliases \
  --build-args "--max-jobs 1" \
  --skip-package-regex 'python3(\d){1,2}Packages\.(pytorch-pfn-extras|ffcv)' \
  --skip-package-regex 'python3(\d){1,2}Packages\.(shap|mlflow|optuna)' \
  --extra-nixpkgs-config "{
    allowUnfree = true;
    allowBroken = false;
    cudaSupport = ${CUDA_SUPPORT:-false};
    cudaCapabilities = ${CUDA_CAPABILITIES:-[]};
  }"

The packages python3Packages.{pytorch-pfn-extras,shap} are excluded because their checkPhase runs for multiple hours -- other excluded packages are dependent on these, and nixpkgs-review does not handle transitive dependencies, so we must find them ourselves.

Template log archival command:

PR=281371; \
RUN_NUMBER=13; \
SYSTEM="aarch64-linux"; \
CUDA_CAPABILITIES="7_5"; \
tar -cvf "$SYSTEM-cap-$CUDA_CAPABILITIES-pr-$PR-$RUN_NUMBER.tar.zst" \
  -I "zstd -T0 --ultra -22" \
  -C "$HOME/.cache/nixpkgs-review/pr-$PR-$RUN_NUMBER" \
  .

Logs are made available at https://drive.google.com/drive/folders/1GgABhwa2ooKeZXMqf5He6PcyClNrE6cQ?usp=share_link

aarch64-darwin

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = false; cudaCapabilities = [ ]; }' run on aarch64-darwin 1

x86_64-darwin

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = false; cudaCapabilities = [ ]; }' run on x86_64-darwin 1

aarch64-linux

✅ CUDA (Jetson)

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = true; cudaCapabilities = [ "7.5" ]; }' run on aarch64-linux 1

✅ CUDA (Non-Jetson)

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = true; cudaCapabilities = [ "7.2" ]; }' run on aarch64-linux 1

✅ Non-CUDA

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = false; cudaCapabilities = [ ]; }' run on aarch64-linux 1

x86_64-linux

✅ CUDA (Non-Jetson)

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = true; cudaCapabilities = [ "7.2" ]; }' run on x86_64-linux 1

✅ Non-CUDA

Result of nixpkgs-review pr 281371 --extra-nixpkgs-config '{ allowUnfree = true; allowBroken = false; cudaSupport = false; cudaCapabilities = [ ]; }' run on x86_64-linux 1

@ConnorBaker ConnorBaker marked this pull request as ready for review January 16, 2024 19:00
@SomeoneSerge
Copy link
Contributor

Interesting! I now wonder what would buildPackages.wrapCCWith do in the useLibsFrom definition

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 17, 2024
@ConnorBaker ConnorBaker marked this pull request as draft January 17, 2024 05:31
@ConnorBaker ConnorBaker force-pushed the feat/stdenvAdapters-useLibsFrom-specify-coreutils branch from 2739623 to 01f2ee0 Compare January 17, 2024 18:04
@ConnorBaker ConnorBaker marked this pull request as ready for review January 17, 2024 18:07
@ConnorBaker ConnorBaker changed the title stdenvAdapters.useLibsFrom: optionally specify coreutils stdenvAdapters.useLibsFrom: use coreutils from targetStdenv Jan 17, 2024
@ConnorBaker
Copy link
Contributor Author

Updated, rebased, and renamed the PR.

@SomeoneSerge can you give it another round of review? I minimized the changes so all this PR does is pass coreutils from targetStdenv.

@ConnorBaker ConnorBaker force-pushed the feat/stdenvAdapters-useLibsFrom-specify-coreutils branch from b4496dc to bf95e27 Compare March 20, 2024 03:28
@ConnorBaker ConnorBaker changed the title stdenvAdapters.useLibsFrom: use coreutils from targetStdenv stdenvAdapters.useLibsFrom: use targetStdenv.cc.override Mar 20, 2024
As @SomeoneSerge pointed out in NixOS#281371 (comment),
by avoiding `wrapCCWith` and using `targetStdenv.cc.override`, we avoid roundtrip wrapping and
are able to use `coreutils` from `targetStdenv`.
@ConnorBaker ConnorBaker force-pushed the feat/stdenvAdapters-useLibsFrom-specify-coreutils branch from 46b1100 to e871fcf Compare March 20, 2024 16:02
@ConnorBaker
Copy link
Contributor Author

@SomeoneSerge I added your comment fix and included a reference to #283517. Can you take another look? Pending your approval and CI passing, I'd like to merge this.

@ConnorBaker ConnorBaker merged commit f6959c2 into NixOS:master Mar 20, 2024
18 checks passed
@ConnorBaker ConnorBaker deleted the feat/stdenvAdapters-useLibsFrom-specify-coreutils branch March 20, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cuda Parallel computing platform and API 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants