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: Fix bazel query and provide a default java toolchain #98611

Merged
merged 2 commits into from Oct 19, 2020

Conversation

@mfarrugi
Copy link
Contributor

@mfarrugi mfarrugi commented Sep 24, 2020

Motivation for this change

These are fixes for problems I ran into with:

  • bazel test //example:cpp-test
    This needed build --host_javabase='@local_jdk//:jdk'

  • bazel query 'deps(//example:cpp-test)'
    This needed the same flags as build. (Trying to get a sense of why the cpp-test needs java!)

Is it contentious to (partially?) configure the default java toolchain? I don't see it as much different than providing the bazel server's java.
It would continue to be configurable/overridable by overriding the flags.
#86410 is related.

(will rewrite commit later, web ui is tricky :))

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

And a random notes from this escapade, but https://github.com/bazelbuild/bazel/blob/master/WORKSPACE#L144-L308 looks a little different from https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/bazel/bazel_3/src-deps.json so one of them is probably wrong :)

These are fixes for problems I ran into with:
- `bazel test //example:cpp-test`
This needed `build --host_javabase='@local_jdk//:jdk'`

- `bazel query 'deps(//example:cpp-test)'`
This needed the same flags as `build`.

Is it contentious to (partially?) configure the default java toolchain? I don't see it as much different than providing the bazel server's java.
It would continue to be configurable/overridable by overriding the flags. 

---
And a random notes from this escapade, but https://github.com/bazelbuild/bazel/blob/master/WORKSPACE#L144-L308 looks a little different from https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/bazel/bazel_3/src-deps.json so one of them is probably wrong :)
@mfarrugi mfarrugi requested a review from Profpatsch as a code owner Sep 24, 2020
@Infinisil
Copy link
Member

@Infinisil Infinisil commented Sep 28, 2020

Could this be related to tweag/rules_haskell#1414? In my quick testing, this PR's bazel didn't fix that issue, so maybe not.

@mfarrugi
Copy link
Contributor Author

@mfarrugi mfarrugi commented Sep 28, 2020

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 8, 2020

@aherrmann does this look familiar to you?

@aherrmann
Copy link
Member

@aherrmann aherrmann commented Oct 9, 2020

The problem with bazel query is that it doesn't take configuration into account. Meaning it won't evaluate selects or apply platform constraints to select a toolchain. The cquery command is an alternative that does take configuration into account. In the rules_haskell/tutorial example it seems to work without error (cc @Infinisil)

$ nix-shell --pure ../shell.nix --run 'bazel cquery --nohost_deps --noimplicit_deps "deps(//main:demorgan)"'
...
//main:demorgan (e05802078203a74d9644c5b7ab2f441c7edc77c7cf9c943d34e190a170d74972)
//lib:booleans (e05802078203a74d9644c5b7ab2f441c7edc77c7cf9c943d34e190a170d74972)
//main:Main.hs (null)
//main:base (e05802078203a74d9644c5b7ab2f441c7edc77c7cf9c943d34e190a170d74972)
//lib:Bool.hs (null)
...

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 9, 2020

So sounds to me like this is a “works as intended” situation? It is confusing, but we can’t really compensate for it.

@mfarrugi
Copy link
Contributor Author

@mfarrugi mfarrugi commented Oct 10, 2020

If you use bazel with internet access, it will download the other platforms' java_tools to resolve the query. Given that nix is doing the fetching, I think this makes sense. The cost to this fix is downloading two extra .zip files that total ~100mb.

I can see the argument that a person shouldn't be using bazel query over bazel cquery, but I see no plans to remove the former and as long as it's there it should work out of the box.

Separately, any thoughts on the --host_javabase change? This seems strictly better for users.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 13, 2020

I am not deep enough into the bazel build anymore to have much of an opinion on whether this change is correct, it is all very confusing and you never know if things have to be there because of a bug in bazel or a works-as-intended.

So I’d say let’s merge this and see what breaks.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 13, 2020

And a random notes from this escapade, but https://github.com/bazelbuild/bazel/blob/master/WORKSPACE#L144-L308 looks a little different from https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/bazel/bazel_3/src-deps.json so one of them is probably wrong :)

I nearly missed this, but I don’t understand what you mean by that? Can you elaborate?

@mfarrugi
Copy link
Contributor Author

@mfarrugi mfarrugi commented Oct 13, 2020

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 13, 2020

@mfarrugi
Copy link
Contributor Author

@mfarrugi mfarrugi commented Oct 13, 2020

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 16, 2020

is parsing the entire WORKSPACE file, so it is indeed a superset. It could probably just read additional_distfiles from bazel, but I'm not interested in pursuing this further.

I’m sorry, I still don’t understand what you mean. What exactly is the problem here?

@mfarrugi
Copy link
Contributor Author

@mfarrugi mfarrugi commented Oct 16, 2020

It's not relevant to this PR.

https://docs.bazel.build/versions/master/guide.html#running-bazel-in-an-airgapped-environment suggests that bazel should only need https://github.com/bazelbuild/bazel/blob/master/WORKSPACE#L144-L308 to work airgapped.  We are doing something different than bazel build @additional_distfiles//:archives.tar that, if the docs are to be believed, are downloading a superset of the necessary files.

@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 19, 2020

@Profpatsch Profpatsch merged commit ffac55a into NixOS:master Oct 19, 2020
19 checks passed
@Profpatsch
Copy link
Member

@Profpatsch Profpatsch commented Oct 19, 2020

Okay, I merged under the assumption that this PR does set a sensible default. We’ll see if any complaints come in.

@mfarrugi mfarrugi deleted the patch-1 branch Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants