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

abuild: put findutils in nativeBuildInputs explicitly #177682

Closed
wants to merge 1 commit into from
Closed

abuild: put findutils in nativeBuildInputs explicitly #177682

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 14, 2022

Description of changes

PR #168413 causes setup.sh to invoke find; this broke abuild, because when setup.sh runs for abuild, $busybox/bin/find appears earlier in $PATH than $findutils/bin/find.

Let's fix this by explicitly adding findutils to nativeBuildInputs, which causes its find to appear earlier in the $PATH.

I am still waiting for my build at staging (97c4382) to get caught up. Edit: build completed, no failures.

However I was able to reproduce the problem that this fixes, and also verify that it is fixed, on my own local tree which is roughly 22.05 plus #168413 plus some other stuff.

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.

Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Seems sensible to me. Other packages may run into this as well, maybe. But in general I'd expect (hope?) build scripts to be somewhat conservative in what tooling features they expect.

@ghost
Copy link
Author

ghost commented Jun 14, 2022

@ofborg build abuild

@ghost
Copy link
Author

ghost commented Jun 14, 2022

Seems sensible to me. Other packages may run into this as well, maybe.

This is a general gripe that I have with nixpkgs' busybox expression. It is the reason why I submitted #161007.

A long-term fix would be to have busybox's symlinks moved to a separate derivation. So the outpath of busybox would contain only bin/busybox and the outpath of busybox-symlinks would have all the bin/find -> $busybox/bin/busybox stuff.

The idea here is:

  • You don't put busybox-symlinks into nativeBuildInputs unless you really want it to clobber coreutils/findutils/etc, which I'm pretty sure is never what you want.
  • You can add busybox-symlinks to buildInputs if the software being built expects to find busybox-provided utilities (e.g. sh) at runtime (for example, recent versions of nix).
  • You can also add busybox-symlinks to your $PATH if you want busybox to be your shell/coreutils/findutils/etc -- for example when building initrds, or in the "nixpkgs bootloader" I write into my SPI flash along with coreboot.

@ghost
Copy link
Author

ghost commented Jun 14, 2022

Another possibility would be for setup.sh to check whether or not find is really findutils' find. I'm not sure what would be the right way to perform this check. grep something $(find --version) || exit -1 seems too fragile. Actually this is probably good enough:

find --version | grep -q 'GNU findutils' || (echo 'the find in $PATH is not findutils'; exit -1)

setup.sh already does this to make sure it's running under bash rather than some other shell.

This won't avoid problems, but it will make their root cause obvious when they arise.

@sternenseemann
Copy link
Member

Should target staging-next if this issue is in fact already present in staging-next.

@ghost
Copy link
Author

ghost commented Jun 15, 2022

Ok. I will draftify, then change branch, then undraftify.

I figured out that is the trick to prevent mass-pinging people when changing the base branch.

@ghost ghost marked this pull request as draft June 15, 2022 19:20
@ghost ghost changed the base branch from staging to staging-next June 15, 2022 19:21
@ghost ghost marked this pull request as ready for review June 15, 2022 19:21
@ghost
Copy link
Author

ghost commented Jun 15, 2022

Should target staging-next if this issue is in fact already present in staging-next.

Done.

Note that if #177789 works as expected (still verifying it -- needs global rebuild) and is merged then this PR not necessary, although it might still be a good idea from a hygiene perspective (packages really should not be fooling setup.sh into using different stdenv-tools).

I should know if #177789 works in about six hours or so.

PR #168413 causes `setup.sh` to invoke `find`; this broke abuild,
because when `setup.sh` runs for abuild, $busybox/bin/find appears
earlier in `$PATH` than `$findutils/bin/find`.

Let's fix this by explicitly adding `findutils` to
`nativeBuildInputs`, which causes its `find` to appear earlier in the
`$PATH`.
@ghost
Copy link
Author

ghost commented Jun 19, 2022

#177789 is a better solution; it covers all packages rather than just this one.

@ghost ghost closed this Jun 19, 2022
@ghost ghost deleted the fix/breakage/pr1618413/abuild branch January 23, 2024 06:45
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants