Skip to content

darwin.stdenv: allow patchShebangs during the bootstrap#242519

Merged
vcunat merged 1 commit intoNixOS:staging-nextfrom
reckenrode:stdenv-shebangs
Jul 10, 2023
Merged

darwin.stdenv: allow patchShebangs during the bootstrap#242519
vcunat merged 1 commit intoNixOS:staging-nextfrom
reckenrode:stdenv-shebangs

Conversation

@reckenrode
Copy link
Copy Markdown
Contributor

@reckenrode reckenrode commented Jul 9, 2023

Description of changes

This fixes pyicu (and any other package that uses icu-config instead of the CMake or some other module to get the build flags).

What happened here is the bootstrap disables patchShebangs to avoid propagating the bootstrap tools to the final stdenv (due to sh and bash being on the PATH from the bootstrap tools). Because of that, the #!/bin/sh line in icu-config was not updated, causing it to invoke the system bash on Darwin. While that is undesirable in its own right, when the system bash is invoked as sh, echo -n will print -n, resulting in the breakage see in #241951 (comment).

The fix is to build bash earlier in the bootstrap while making sure it is picked up over the one in the bootstrap tools. That allows patchShebangs to be enabled during the bootstrap. Any package with scripts that is included in the final stdenv should now have its scripts’ shebang lines properly patched.

I tested that pyicu builds, and this is the contents of icu-config now:

$ nix build .#icu^dev
$ head -n 1 ./result-dev/bin/icu-config
#!/nix/store/znf9b5c859lgz348r200pgi0y9855y1l-bash-5.2-p15/bin/sh
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/)
  • 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.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jul 9, 2023
@reckenrode
Copy link
Copy Markdown
Contributor Author

Creating this as a draft PR while x86_64-darwin builds. I feel pretty confident that it will, but I don’t want to waste time on Hydra if it doesn’t. Once the stdenv is built, I’ll set the PR to ready.

@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin 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: 0 This PR does not cause any packages to rebuild on Linux. labels Jul 9, 2023
This fixes pyicu (and any other package that uses `icu-config` instead
of the CMake or some other module to get the build flags).

What happened here is the bootstrap disables `patchShebangs` to avoid
propagating the bootstrap tools to the final stdenv (due to `sh` and
`bash` being on the `PATH` from the bootstrap tools). Because of that,
the `#!/bin/sh` line in `icu-config` was not updated, causing it to
invoke the system bash on Darwin. While that is undesirable in its own
right, when the system bash is invoked as `sh`, `echo -n` will print
`-n`, resulting in the breakage see in NixOS#241951 (comment).

The fix is to build bash earlier in the bootstrap while making sure it
is picked up over the one in the bootstrap tools. That allows
`patchShebangs` to be enabled during the bootstrap. Any package with
scripts that is included in the final stdenv should now have its
scripts’ shebang lines properly patched.
@reckenrode reckenrode marked this pull request as ready for review July 10, 2023 00:57
@reckenrode
Copy link
Copy Markdown
Contributor Author

Setting this to review. I had to build link bash against the system CoreFoundation to break a dependency cycle on x86_64-darwin.

I was able to successfully build a stdenv on both x86_64-darwin and aarch64-darwin, and I was able to build pyicu on aarch64-darwin. I feel pretty confident pyicu will build on x86_64-darwin, but I don’t want to delay longer than necessary.

Copy link
Copy Markdown
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

LGTM. These cycles are crazy!

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jul 10, 2023

The failure in texlive.bin.core-big is fixed by this as well? The failure does happen soon after running icu-config, but the error messages are weird. Could this be cheap for you to run, as you have stuff in cache?

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jul 10, 2023

To be clear, Hydra showed the failure the same on both *-darwin, so again it should suffice to verify either for now.

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jul 10, 2023

Ah, I now caught up the confirmation for those questions in another thread: #242202 (comment)

@vcunat vcunat merged commit f855ba2 into NixOS:staging-next Jul 10, 2023
@vcunat
Copy link
Copy Markdown
Member

vcunat commented Jul 10, 2023

OK, the icu issue seems quite serious, so I'll be optimistic and merge fast. (without reviewing myself; I'm mostly avoiding darwin)

@vcunat vcunat mentioned this pull request Jul 10, 2023
1 task
@reckenrode
Copy link
Copy Markdown
Contributor Author

Sorry about the delayed response. I was asleep at the time these comments were posted.

The failure in texlive.bin.core-big is fixed by this as well?

If it’s due to icu, then I expect it to be fixed. The icu-config script was messed up. This PR also fixes any other scripts (like xml2-config from libxml2) that were provided by the derivations built during the bootstrap.

The failure does happen soon after running icu-config, but the error messages are weird. Could this be cheap for you to run, as you have stuff in cache?

I’m building it now and will report back.

@reckenrode
Copy link
Copy Markdown
Contributor Author

@vcunat Both texlive and R build for me with this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 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: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants