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: use six X characters in mktemp invocation #178626

Merged
merged 1 commit into from Jun 23, 2022
Merged

setup.sh: use six X characters in mktemp invocation #178626

merged 1 commit into from Jun 23, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 22, 2022

Description of changes

Closes #178625

The busybox version of mktemp requires exactly six X characters in the argument to mktemp, unlike the coreutils version of mktemp.

Let's accomodate packages, like epson-escpr2, which fool setup.sh into using the busybox version instead of the stdenv version.

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.

Closes #178625

The `busybox` version of `mktemp` requires exactly six `X` characters
in the argument to `mktemp`, unlike the `coreutils` version of `mktemp`.

Let's accomodate packages, like `epson-escpr2`, which fool `setup.sh`
into using the `busybox` version instead of the `stdenv` version.
@Mindavi
Copy link
Contributor

Mindavi commented Jun 23, 2022

https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/mktemp.html

The template should have at least six trailing 'X' characters. These characters are replaced with characters from the portable filename character set in order to generate a unique name.

Looks good to me. I like that stdenv is made to be very portable, that will make porting easier if some other shell is used in the future.

@Mindavi Mindavi merged commit adafa1c into NixOS:staging Jun 23, 2022
@ghost
Copy link
Author

ghost commented Jun 23, 2022

https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/mktemp.html

The template should have at least six trailing 'X' characters.

One of the great parts about contributing to nixpkgs is learning unexpected stuff like this :)

I like that stdenv is made to be very portable, that will make porting easier if some other shell is used in the future.

Well, technically coreutils is part of stdenv... in fact that was the main reason why I did not test my changes on any other shell or "utils".

I think we ought to alert people to the fact that setup.sh currently works with busybox, and there are packages which depend on this behavior.

One way to do this is to make sure that applications/misc/hello/default.nix fails to build if people don't preserve this behavior. For example, hello deliberately passes a function (rather than an attrset) to mkDerivation so people are forced to make sure they don't break that capability. Unfortunately making hello use busybox to execute setup.sh would add an unnecessary and large dependency to hello, so that isn't a good idea.

Any ideas on how else to make sure people notice if they make changes to setup.sh that don't work with busybox?

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.

2 participants