treewide: skip generating shell completions using $out/bin/… when cross compiling#326243
Merged
pbsds merged 6 commits intoNixOS:masterfrom Aug 6, 2024
Merged
treewide: skip generating shell completions using $out/bin/… when cross compiling#326243pbsds merged 6 commits intoNixOS:masterfrom
pbsds merged 6 commits intoNixOS:masterfrom
Conversation
5e9658d to
3bb551e
Compare
pbsds
reviewed
Aug 3, 2024
Member
pbsds
left a comment
There was a problem hiding this comment.
diff LGTM sans one error, i also support this approach until someone rather decides to go the QEMU route and puts the work into actually implementing it
pkgs/tools/misc/starship/default.nix
Outdated
Member
There was a problem hiding this comment.
the presets below are not conditioned on canExecute
Contributor
Author
There was a problem hiding this comment.
Nice catch. Fixed.
Also rebased on master, and additionally had to fix the formatting of maa-cli now to get a green CI…
|
Result of 3 packages built:
|
upstream recommends using cargo xtask gen, but checks in the results
…ss compiling
This focuses on Rust packages, since the most commonly used argument
parser library (clap/structopt) makes the following pattern natural and
thus common:
postInstall = ''
installShellCompletion --cmd foo \
--bash <($out/bin/foo completion bash) \
…
This commit just guards those with
lib.optionalString (stdenv.buildPlatform.canExecute stdenv.hostPlatform)
splitting the string where unrelated actions are performed.
…ecute This rewrites uses of stdenv.hostPlatform == stdenv.buildPlatform to stdenv.buildPlatform.canExecute stdenv.hostPlatform when guarding postInstall scripts that use $out/bin/… to generate shell completions
|
Result of 4 packages built:
|
pbsds
approved these changes
Aug 6, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Fixes regressions for cross compiling introduced in #289517 on 61 packages. See #308283 for more discussion.
There are a lot of packages that use the following pattern to generate shell completions:
The crucial part is that it executes
$out/bin/foo, which failed silently on cross builds so far (ignoring binfmt_misc here), but is now raised as a build error frominstallShellCompletion.Most of what this PR does is prefix (that part of) those
postInstallscripts with:Some additional notes:
buildRustPackage, because I'm familiar with that ecosystem, and the completion generation logic is nearly all the same, so it was easy to investigate problems for me.buildPlatform == hostPlatformtobuildPlatform.canExecute hostPlatform.Checking
nixpkgs-review isn't super insightful for this, since I took extra care to not introduce any changes, not even whitespace, where unnecessary:
Result of
nixpkgs-reviewrun on x86_64-linux 13 packages built:
So, I also obtained a list of changed packages
and tested that all build, with:
(skipping maa-cli sheesy-cli deno tremor solana-cli elf2nucleus because they fail to compile or link.)
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.