Skip to content

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

Merged
ConnorBaker merged 1 commit intoNixOS:masterfrom
ConnorBaker:feat/stdenvAdapters-useLibsFrom-specify-coreutils
Mar 20, 2024
Merged

stdenvAdapters.useLibsFrom: use targetStdenv.cc.override#281371
ConnorBaker merged 1 commit intoNixOS:masterfrom
ConnorBaker:feat/stdenvAdapters-useLibsFrom-specify-coreutils

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
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels 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
@github-actions github-actions bot removed the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 17, 2024
@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
@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