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

setup.sh: verify that find is from findutils #177693

Closed
wants to merge 3 commits into from
Closed

setup.sh: verify that find is from findutils #177693

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 14, 2022

Description of changes

PR #168413 causes setup.sh to use find. Packages which put busybox into their nativeBuildInputs will have $busybox/bin/find ahead of $findutils/bin/find in $PATH when setup.sh is run, causing setup.sh to fail.

Let's check that find is really from findutils so we can provide a more useful error message.

The setup.sh script already does a similar check to make sure that the shell it is running under is bash rather than some other shell.

A longer-term solution to the problem of busybox's outpaths conflicting with stdenvs is described here:

#177682 (comment)

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.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

PR #168413 causes `setup.sh` to use `find`.  Packages which put
`busybox` into their `nativeBuildInputs` will have `$busybox/bin/find`
ahead of `$findutils/bin/find` in `$PATH` when `setup.sh` is run,
causing `setup.sh` to fail.

Let's check that `find` is really from findutils so we can provide a
more useful error message.

The `setup.sh` script already does a similar check to make sure that
the shell it is running under is `bash` rather than some other shell.

A longer-term solution to the problem of `busybox`'s outpaths
conflicting with `stdenv`s is described here:

  #177682 (comment)
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jun 14, 2022
Adam Joseph and others added 2 commits June 14, 2022 23:07
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@@ -1022,6 +1022,15 @@ configurePhase() {
fixLibtool "$i"
done

# Make sure `find` is the full `find` from findutils, rather
# than from busybox or some other implementation. This is
# analogous to `shellcheck shell=bash` at the top of this
Copy link
Contributor

Choose a reason for hiding this comment

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

shellcheck is a tool to verify if shell scripts are valid, it's unrelated to checking if the correct shell is used.

Copy link
Author

Choose a reason for hiding this comment

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

:embarrassed:

@Mindavi
Copy link
Contributor

Mindavi commented Jun 15, 2022

Could there be reasons for people to want to use find from busybox? This seems quite strict, though I understand where you're coming from.

@alyssais
Copy link
Member

What features of GNU findutils do we need that Busybox's find doesn't have? Would it be possible to make setup.sh compatible with both?

@Mindavi
Copy link
Contributor

Mindavi commented Jun 15, 2022

Hmm, I thought it was abuild that was expecting find with printf. I don't see -printf in the fixLibtool PR.

So putting it here may be at the wrong layer, but not sure where to put it then. And I agree we don't want conflicting binaries in stdemv regardless.

@ghost
Copy link
Author

ghost commented Jun 15, 2022

What features of GNU findutils do we need that Busybox's find doesn't have?

Just -execdir, which I used out of habit. It isn't needed here because TOCTOU attacks on nix's builders' temporary directories (/tmp/nix-build-*) shouldn't be possible.

Could there be reasons for people to want to use find from busybox?

I'm not sure. But it can't hurt.

Would it be possible to make setup.sh compatible with both?

Sure: #177789

@ghost ghost closed this Jun 15, 2022
@ghost
Copy link
Author

ghost commented Jun 15, 2022

Hmm, I thought it was abuild that was expecting find with printf

It's the -execdir that busybox find doesn't like.

And I agree we don't want conflicting binaries in stdemv regardless.

I am certainly on board with using the minimum features needed for the task, so I'm switching this to -exec. But yeah in general I hope this isn't seen as encouraging people to expect that nixpkgs will work effortlessly if they swap out pieces of stdenv for alternatives with different feature sets. Like say the OpenBSD userspace versions of all our GNU tools... that would definitely break a lot of stuff.

@alyssais
Copy link
Member

It would be really nice if build inputs looked at meta.priority, IMO. There are other situations where it'd be nice — e.g. I have a shell.nix with both virtiofsd and qemu, and I have to make sure to specify them in the right order so I don't get QEMU's deprecated built-in virtiofsd ahead in PATH.

@ghost
Copy link
Author

ghost commented Jun 15, 2022

It would be really nice if build inputs looked at meta.priority

Hrm, is that possible?

I thought that changing a package's meta would not affect its drv-hash (or the drv-hashes of packages which depend on it).

If builders could see their inputs' meta.priority then changes to meta.priority would affect the build process, and therefore would have to affect the drv-hash.

@ghost
Copy link
Author

ghost commented Jun 15, 2022

Also while we're wishing for things, I wish meta.priority were a partial ordering rather than a total non-dense-ordering.

So for any given package you can add a list of other packages it is strictly-less-than (or strictly-greater than). No integers to remember and no running out of priority levels between e.g. "5" and "6".

I can never seem to remember integer priority systems like the one in nftables.

@ghost ghost deleted the pr/setup/checkfind branch January 23, 2024 06:50
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