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

Jump to bottom

bazel: Fix bazel query and provide a default java toolchain #98611

Merged
merged 2 commits into from Oct 19, 2020
Merged

bazel: Fix bazel query and provide a default java toolchain #98611

merged 2 commits into from Oct 19, 2020

Conversation

mfarrugi
Copy link
Contributor

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 :)

@mfarrugi
bazel: Fix bazel query and provide a default java toolchain
b553605 
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 mentioned this pull request Sep 24, 2020
Open
@ofborg ofborg bot requested a review from mboes September 24, 2020 03:39
@infinisil
Copy link
Member

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 commented Sep 28, 2020 via email

@Profpatsch
Copy link
Member

@aherrmann does this look familiar to you?

@aherrmann
Copy link
Member

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

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

@Profpatsch Profpatsch mentioned this pull request Oct 9, 2020
Closed
@mfarrugi
Copy link
Contributor Author

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

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

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 commented Oct 13, 2020 via email

@Profpatsch
Copy link
Member

Profpatsch commented Oct 13, 2020 via email

@mfarrugi
Copy link
Contributor Author

mfarrugi commented Oct 13, 2020 via email

@Profpatsch
Copy link
Member

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

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 commented Oct 19, 2020 via email

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

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 October 19, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Profpatsch Profpatsch Awaiting requested review from Profpatsch Profpatsch is a code owner

@mboes mboes Awaiting requested review from mboes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mfarrugi @infinisil @Profpatsch @aherrmann @Janik-Haag