-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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: 0.18.0 -> 0.20.0 #51748
bazel: 0.18.0 -> 0.20.0 #51748
Conversation
We have a second PR in #51783, which only adds one new line. |
--host_javabase=@bazel_tools//tools/jdk:absolute_javabase \ | ||
--define=ABSOLUTE_JAVABASE=$JAVA_HOME \ | ||
--host_java_toolchain=@bazel_tools//tools/jdk:toolchain_vanilla \ | ||
--java_toolchain=@bazel_tools//tools/jdk:toolchain_vanilla \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these lines do? It looks similar to what https://github.com/NixOS/nixpkgs/pull/51783/files#diff-4746b81564a01e241a84d455575437e7R191 does in one line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--server_javabase
set the java version used for the Bazel server, --host_javabase
sets the java version used for the worker processes, --*java_toolchain
sets the toolchain used to build java code.
I'm surprised setting --server_javabase
is enough, maybe the other two default to the value passed for that.
Why I pass it separately here is that in the checkPhase
we haven't wrapped the program yet (#51783 deletes checkPhase
so it doesn't have that issue).
Yeah #51783 avoids a lot of complexity by deleting some stuff, I think that's good assuming we're ok with deleting it. |
Oh, hm, not a fan of deleting tests like these. Of course for a tool like bazel that takes half an hour to build, it’s infuriating if some ad-hoc tests fail at the very end. |
@mboes What do you think? I'm happy for either one to go through. |
sed -i -e "421 a --copt=\"$(echo $NIX_CFLAGS_COMPILE | sed -e 's/ /" --copt=\"/g')\" \\\\" scripts/bootstrap/compile.sh | ||
sed -i -e "421 a --host_copt=\"$(echo $NIX_CFLAGS_COMPILE | sed -e 's/ /" --host_copt=\"/g')\" \\\\" scripts/bootstrap/compile.sh | ||
sed -i -e "421 a --linkopt=\"-Wl,$(echo $NIX_LDFLAGS | sed -e 's/ /" --linkopt=\"-Wl,/g')\" \\\\" scripts/bootstrap/compile.sh | ||
sed -i -e "421 a --host_linkopt=\"-Wl,$(echo $NIX_LDFLAGS | sed -e 's/ /" --host_linkopt=\"-Wl,/g')\" \\\\" scripts/bootstrap/compile.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be BAZEL_BOOTSTRAP_STARTUP_OPTIONS on https://github.com/bazelbuild/bazel/blob/be2fd95b29e55eb5b300635b82610e35be7f0040/scripts/bootstrap/compile.sh#L433
used instead sed commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should replace those lines by something sane. Sadly, the bootstrap scripts are a bit wild and hard to patch correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we can set that environment variable when calling the script, that's a very good find, not sure how I missed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried it but it doesn't work because this is only for startup options: options that go before the command, and are parsed by the bazel client, and not regular options, which go after the command and are parsed by the bazel server.
I merged #51783, because it removes more lines than it introduces (by disabling a superfluous test). |
@uri-canva could you make a new pull request that switches to |
Motivation for this change
Upgrading Bazel. Tested
bazel
,bazel_jdk11
,bazel-watcher
,bazel-deps
,pythonPackages.tensorflow
,pythonPackages27.tensorflow
,pythonPackages36.tensorflow
.bazel-watcher
requires #51723.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)