Skip to content

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

Merged
Mindavi merged 1 commit intostagingfrom
unknown repository
Jun 23, 2022
Merged

setup.sh: use six X characters in mktemp invocation#178626
Mindavi merged 1 commit intostagingfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Jun 22, 2022 •
edited by ghost
Loading

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.

@ghost ghost requested review from Ericson2314 and matthewbauer as code owners June 22, 2022 19:45
@ghost ghost mentioned this pull request Jun 22, 2022
Merged
9 tasks
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jun 22, 2022
setup.sh: use six X characters in mktemp invocation
97bf76b 
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.
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jun 22, 2022
@Mindavi
Copy link
Contributor

Mindavi commented Jun 23, 2022 •
edited
Loading

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?

@trofi trofi mentioned this pull request Jun 25, 2022
Closed
@Shawn8901 Shawn8901 mentioned this pull request Jun 26, 2022
Merged
13 tasks
@Artturin Artturin mentioned this pull request Jul 19, 2023
Merged
12 tasks
@Artturin Artturin mentioned this pull request Aug 1, 2023
Merged
12 tasks
@ghost ghost deleted the pr/168413fallout/mktemp branch January 23, 2024 06:47
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Mindavi @sternenseemann