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.

Sign up for GitHub

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

Jump to bottom

bash: avoid using unspliced dependency #265162

Closed
wants to merge 3 commits into from
Closed

bash: avoid using unspliced dependency #265162

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2023 •
edited by ghost
Loading

Description of changes

stdenv is spliced, but stdenv.cc is not, so we can't use the
latter in dependencies.

Closes

Includes

Fixes #243164

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/)
  • 23.11 Release Notes (or backporting 23.05 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.

Adam Joseph added 3 commits November 2, 2023 23:58
lib.systems, test.cross.sanity: add test case for #264989
a7d0caa 
We have several cross-compilation bugs that show up if
hostPlatform!=buildPlatform yet
hostPlatform.config==buildPlatform.config.

These bugs have appeared and disappeared as we've fiddled with the
definition of equality for platform objects.  This commit adds a
clear-cut case where they are *not* equal and never will be, so we
can test it.
gcc.libgcc: compare host and target platforms, rathern than their tri…
8408127 
…ples

The rest of our gcc expression prepends "${targetPlatform.config}-"
to paths and binaries if `hostPlatform!=targetPlatform`.  The
`libgcc.nix` expression was using
'hostPlatform.config!=targetPlatform.config`, which caused it to
look in the wrong place when moving files.  This commit corrects that.
bash: avoid using unspliced dependency
667a1ad 
`stdenv` is spliced, but `stdenv.cc` is not, so we can't use the
latter in dependencies.

Closes #264989
Closes #243164
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 3, 2023
@ghost ghost mentioned this pull request Nov 3, 2023
Merged
13 tasks
@ghost ghost marked this pull request as ready for review November 3, 2023 07:03
@ghost ghost requested review from alyssais and Ericson2314 as code owners November 3, 2023 07:03
@ofborg ofborg bot requested a review from dtzWill November 3, 2023 09:04
@alyssais
Copy link
Member

alyssais commented Nov 3, 2023

stdenv is spliced, but stdenv.cc is not, so we can't use the
latter in dependencies.

I'm a bit confused what you mean by this — are you saying buildPackages.stdenv.cc wasn't getting us what we expected? Should it just have been pkgsBuildBuild.stdenv.cc, since it was in depsBuildBuild?

@ghost
Copy link
Author

ghost commented Nov 3, 2023

I'm a bit confused what you mean by this — are you saying buildPackages.stdenv.cc wasn't getting us what we expected?

Correct.

Try changing buildPackages.stdenv.cc to assert buildPackages.stdenv.cc?__spliced; buildPackages.stdenv.cc and you'll get an assertion.

Should it just have been pkgsBuildBuild.stdenv.cc, since it was in depsBuildBuild?

No; you should not use pkgsFooBar anywhere in depsFooBar (or buildInputs or nativeBuildInputs) because pkgsFooBar are unspliced packages.

See:

which adds a lib.warn when people do this. I hope to merge that right after 23.11 branch-off.

@ghost ghost self-assigned this Nov 4, 2023
@ghost ghost marked this pull request as draft November 4, 2023 01:53
This was referenced Nov 4, 2023
Open
Merged
Artturin
Artturin reviewed Nov 6, 2023
View reviewed changes
pkgs/shells/bash/5.nix
@@ -91,7 +91,7 @@ stdenv.mkDerivation rec {

strictDeps = true;
# Note: Bison is needed because the patches above modify parse.y.
depsBuildBuild = [ buildPackages.stdenv.cc ];
depsBuildBuild = [ stdenv ];
Copy link
Member

@Artturin Artturin Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this will work? how would stdenv become stdenv.cc?
if it works then it shouldn't work.

Copy link
Author

Choose a reason for hiding this comment

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

You're right; this works "by accident".

@Artturin Artturin mentioned this pull request Nov 6, 2023
Closed
13 tasks
@ghost ghost mentioned this pull request Nov 6, 2023
Merged
12 tasks
@ghost
Copy link
Author

ghost commented Nov 6, 2023

This only works "by accident".

@ghost ghost closed this Nov 6, 2023
@ghost ghost deleted the pkgsCross-sanity-gnu64-bash-lastcommit branch November 6, 2023 09:25
@Artturin Artturin mentioned this pull request Nov 25, 2023
Open
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alyssais alyssais Awaiting requested review from alyssais alyssais is a code owner

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@dtzWill dtzWill Awaiting requested review from dtzWill

Assignees
No one assigned
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alyssais @Artturin