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.

Sign up for GitHub

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

Jump to bottom

bazel: patch runfiles.bash to include defaultShellPath in PATH #44079

Merged
merged 1 commit into from Aug 17, 2018
Merged

bazel: patch runfiles.bash to include defaultShellPath in PATH #44079

merged 1 commit into from Aug 17, 2018

Conversation

kalbasit
Copy link
Member

@kalbasit kalbasit commented Jul 25, 2018 •
edited

Motivation for this change

rules_typescript invokes a shell without passing the use_default_shell_env=True and this ends up not using the custom bash that we create for Bazel.

This PR patches runfiles.bash to include the defaultShellPath that we create to the known PATH.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

closes #43955

cc @mboes @Profpatsch

@kalbasit kalbasit mentioned this pull request Jul 25, 2018
Closed
@Profpatsch
Copy link
Member

Thanks, I will look over this later.

@kalbasit
Copy link
Member Author

@Profpatsch rules_typescript was fixed upstream to use the default shell in bazelbuild/rules_typescript@83c491d But I still think we should go for this patch as it could be affecting other rules that I'm not aware of at this time.

@kalbasit
Copy link
Member Author

@Profpatsch do you have some time to get this merged?

@Profpatsch
Copy link
Member

I wouldn’t add workarounds for potentially breaking stuff, especially if the original upstream issue has been fixed.

@mboes
Copy link
Contributor

mboes commented Aug 3, 2018

@Profpatsch no I think @kalbasit's fix is the right way forward. rules_typescript was just one example of something that was broken, but others will hit the same problem on other rule sets without this patch.

@matthewbauer
Copy link
Member

matthewbauer commented Aug 16, 2018 •
edited

@GrahamcOfBorg build bazel

@GrahamcOfBorg
Copy link

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: v

Partial log (click to expand)

Cannot nix-instantiate `v' because:
error: attribute 'v' in selection path 'v' not found

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: v

Partial log (click to expand)

Cannot nix-instantiate `v' because:
error: attribute 'v' in selection path 'v' not found

@GrahamcOfBorg
Copy link

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: bazel

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnfree = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnfree = true; }
to ~/.config/nixpkgs/config.nix.

@GrahamcOfBorg
Copy link

No attempt on x86_64-linux (full log)

The following builds were skipped because they don't evaluate on x86_64-linux: v

Partial log (click to expand)

Cannot nix-instantiate `v' because:
error: attribute 'v' in selection path 'v' not found

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: bazel

Partial log (click to expand)

//examples/cpp:hello-success_test                                        PASSED in 0.1s
//examples/java-native/src/test/java/com/example/myproject:hello         PASSED in 0.5s

Executed 2 out of 2 tests: 2 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
INFO: Build completed successfully, 20 total actions
installing
post-installation fixup
patching script interpreter paths in /nix/store/mlf1c0xxgbjpa4bsf5gf3q9pf4fjvfv6-bazel-0.15.2
/nix/store/mlf1c0xxgbjpa4bsf5gf3q9pf4fjvfv6-bazel-0.15.2

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: bazel

Partial log (click to expand)

//examples/java-native/src/test/java/com/example/myproject:hello         PASSED in 0.6s

Executed 2 out of 2 tests: 2 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
INFO: Build completed successfully, 22 total actions
installing
post-installation fixup
patching script interpreter paths in /nix/store/lnj8yjlhvapswyvxc47ibbjdakapx1yk-bazel-0.15.2
checking for references to /tmp/.bazel-1000 in /nix/store/lnj8yjlhvapswyvxc47ibbjdakapx1yk-bazel-0.15.2...
/nix/store/lnj8yjlhvapswyvxc47ibbjdakapx1yk-bazel-0.15.2

@Profpatsch Profpatsch merged commit 9c5b7cc into NixOS:master Aug 17, 2018
@kalbasit kalbasit deleted the fix-bazel-runfiles.bash branch August 22, 2018 23:41
@infinisil infinisil mentioned this pull request Mar 29, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bazel with rules_typescript does not work
5 participants
@kalbasit @Profpatsch @mboes @matthewbauer @GrahamcOfBorg