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

bazel_6: fix: make patched bash a native binary #268614

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

boltzmannrain
Copy link
Contributor

@boltzmannrain boltzmannrain commented Nov 20, 2023

Description of changes

https://hydra.nixos.org/build/240805256/nixlog/1
https://hydra.nixos.org/build/240805170/nixlog/2
Failure is a bit obscured but long story short, a script within bazel gets custom nixpkgs shebang which in turn makes shell run in POSIX-compatible mode. Bazel expects bash in non-POSIX mode and osx-specific script starts to fail due to set -e and subshell interaction differences in those modes (sub-shells and functions suddently start inheriting set -e and fail to produce desired output). More debug info is available in #267670

Shell scripts aren't guaranteed to work as interpreters in shebang. In particular thin shell wrappers aren't shebang-ready on MacOS. It may work sometimes depending on what exactly would try to execute a script with such shebang, but generally it's not guaranteed to work. See #124556

Bash wrapper was introduced in #266847 and so far seems like the issue only affects darwin builds: hydra failure is in osx-specific script, also shebang issue is usually darwin-specific.

Let's wrap it as a native binary to make it shebang-compatible.

The wrapper is only currently added to bazel_6 so no need for changes in other versions.

Note: there seems to be some other breakage on the most recent main branch, probably makes sense to address separately

ZHF: #265948

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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.

https://hydra.nixos.org/build/240805256/nixlog/1
https://hydra.nixos.org/build/240805170/nixlog/2
Failure is a bit obscured but long story short, a script within
bazel gets custom nixpkgs shebang which in turn makes shell run
in POSIX-compatible mode. Bazel expects bash in non-POSIX mode
and osx-specific script starts to fail due to `set -e` and subshell
interaction differences in those modes (sub-shells and functions
suddently start inheriting `set -e` and fail to produce desired
output). More debug info is available in NixOS#267670

Shell scripts aren't guaranteed to work as interpreters in shebang.
In particular thin shell wrappers aren't shebang-ready on MacOS.
It may work sometimes depending on what exactly would try to execute
a script with such shebang, but generally it's not guaranteed to work.
See NixOS#124556

Bash wrapper was introduced in NixOS#266847 and so far seems like the
issue only affects darwin builds: hydra failure is in osx-specific
script, also shebang issue is usually darwin-specific.

Let's wrap it as a native binary to make it shebang-compatible.

The wrapper is only currently added to `bazel_6` so no need for
changes in other versions.

ZHF: NixOS#265948
@boltzmannrain boltzmannrain added the 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign label Nov 20, 2023
Copy link
Contributor

@uri-canva uri-canva left a comment

Choose a reason for hiding this comment

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

I feel like this could be done with writeCBin, but let's unblock for now.

@uri-canva
Copy link
Contributor

Will wait for ofborg builds then merge.

@boltzmannrain
Copy link
Contributor Author

For other issues on recent main branch seems like recent switch to CLang 16 is to blame, have a draft #268620 but it needs more work, turns out we can't override -Werror in upb dependency in 1-liner with common cflags

@uri-canva uri-canva merged commit 8c47608 into NixOS:master Nov 22, 2023
13 checks passed
Copy link
Contributor

github-actions bot commented Dec 7, 2023

Successfully created backport PR for release-23.11:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign 10.rebuild-darwin: 1-10 10.rebuild-linux: 101-500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants