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

darwin.stdenv: allow patchShebangs during the bootstrap #242519

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

reckenrode
Copy link
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
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.

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
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
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
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
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
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
5 checks passed
@vcunat
Copy link
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
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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants