-
-
Notifications
You must be signed in to change notification settings - Fork 14.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 7.1 -> 7.3 #338264
bazel 7.1 -> 7.3 #338264
Conversation
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.
This is the only new patch introduced here. The others are all copied straight from the existing bazel_7
drv.
I'm going to hold off on the by-name migration until we come to a decision about whether or not to replace the existing |
This comment was marked as resolved.
This comment was marked as resolved.
pkgs/development/tools/build-managers/bazel/bazel_73/default.nix
Outdated
Show resolved
Hide resolved
Is the reason to have this as a separate package just getting fetching to work properly? If that can be solved it seems better to update the main Bazel package rather than carrying around multiple versions indefinitely. Nits:
|
Thank you, I realize I didn't really provide any specifics here. Changing how deps are fetched shouldn't be a big deal normally, but we are giving up a good bit of control over resolution and fetching, and I don't fully trust that the new method will remain correct or stable over time. The previous method of fetching used to be able to resolve everything necessary to build the package outside of using Bazel directly. Unfortunately, some changes to how Bazel resolves deps broke our method of resolution and fetching essentially for good. This comment has a summary of the problem and links to a more detailed discussion. Bazel does not guarantee reproducibility in its fetches, so using Bazel to fetch its deps means taking on some level of risk. I don't want to preclude that this is stable enough to use going forward, and I don't want a problem to mean that we no longer have a working Bazel 7 package. These concerns might not be enough to warrant having a separate package, but I wanted to make them known and get others' perspectives.
👍 I will make these changes if we end up sticking with a new package here. |
No worries, I’m not keeping track of Bazel so I’m just missing the context here :) Seems like we’ll have to bite the bullet at some point, right, unless Bazel 7.1 is going to remain a viable platform forever? Or someone else could write a deterministic fetcher tailored to our purposes; I know that’s been done for Node.js. |
Your perspective even without the additional context is valuable, because it tells me this probably does not clear the bar for needing to split the package further. I'm now leaning toward upgrading the existing package in place. Bazel tends to do big feature development in minor versions while gating them behind flags that are disabled by default, and a lot of major version changes are simply flipping the flags to enable the feature by default, so minor versions can come with big feature updates on their own. It would be okay for us to have separate minor versions, I believe, if that were something we absolutely needed to do, but I'm more certain that we don't need to do it in this case. I think a better fetcher might be a preferable solution in the long run simply because we care a lot more about the reproducibility of deps, but I thought it was more important to have a working Bazel 7.3 since we had to skip over 7.2. It's also possible that we see less volatility with the deps for this package in particular, but I am also thinking about a way forward for things like |
It’s okay to maintain separate versions for a time if required to do a migration. For instance, are there existing in‐tree packages using Bazel that would be broken by the fetcher changes (e.g. because they have dependency hashes that would be invalidated)? In that case it’d definitely be best to add another version for now until we can sort things out. If everything in Nixpkgs should otherwise keep working, I think it’s okay to go from one suboptimal situation to another, perhaps with a release note. (And yeah, I’m a big FOD blob hater, personally! But they can be hard to forgo without checking huge lock files into Nixpkgs, depending on the ecosystem…) |
I share that sentiment.
I think a fetcher that is stable over many Bazel versions requires upstream support. Ideally a built in feature like |
bf0c877
to
7b90c79
Compare
I think these changes are pretty well localized to the Bazel derivation itself, so I do think this is fine to upgrade in place. I've switched to Bazel 7.3.1 and did not find any issues after updating the hashes. I'm going to mark this as ready for review while recognizing that the tests have not been updated yet. |
@@ -117,8 +100,126 @@ let | |||
unzip | |||
which | |||
zip | |||
runJdk |
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.
this might not be necessary (was from an attempt to wrap the bootstrap Bazel with JAVA_HOME
)
]; | ||
|
||
# Bootstrap an existing Bazel so we can vendor deps with vendor mode |
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.
This is the salient portion of the changes made to get this working with Bazel 7.3 via vendor mode.
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.
Were we using a binary bootstrap before, or is this a new change? I thought that Bazel always required itself to bootstrap, so it’d help me if you could explain the differences from the previous situation.
Currently it seems like this would restrict Bazel to x86_64-linux
only, which would be unfortunate. Would keeping the old Bazel around to build this one provide us a source‐based bootstrap path, or would we just be using a different binary bootstrap?
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.
Bazel does require itself to bootstrap as part of its own build, but we now need a version of Bazel 7.3 that we can use prior to building Bazel 7.3 in order to be able to run bazel vendor for the FOD that fetches the deps.
We should be able to bootstrap all combinations of darwin/linux and x86_64/arm64. We can certainly do that for linux, though I don't have an arm64-linux machine readily available. My understanding with the darwin derivation is that it builds but is essentially unusable, but I don't have the experience to confirm this. I'd be okay with delisting darwin support as long as people with this experience can verify that understanding.
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.
One thing I'm not certain about: we need the bootstrap without an existing Bazel >= 7.3, but is there a way to get rid of it once we've done an initial bootstrap?
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.
This now should work for all supported platforms, though I haven't tested it on all of them yet.
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.
Thanks, I understand better now. Am I correct in thinking that previously we did not need a binary blob bootstrap Bazel and now we do, and that there is no way to use the old from‐source Bazel to bootstrap the new one? That is unfortunate, but not something we really have control over. You should set at least meta.sourceProvenance
on the bootstrap Bazel accordingly.
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.
That is correct, at least for the approach I've taken to vendor the deps. However, once we have a working Bazel 7.3 cached, is there a way for us to use that instead? It's not obvious to me how, but I figured there might be a way to eliminate the external bootstrap after the fact.
You should set at least meta.sourceProvenance on the bootstrap Bazel accordingly.
done 6338bd3
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.
We have the bootstrap tools tarballs, but those are for things that are required to rebuild the standard environment. I think it wouldn’t make much sense in this case, because the provenance would be essentially the same: we’re trusting the opaque blob to produce a new opaque blob for us, so there’s no point pretending that “laundering” it through Hydra produces something we can confidently say was bootstrapped cleanly from the original source code.
@ofborg build bazel_7 |
, enableNixHacks ? false | ||
, version ? "7.1.2" | ||
enableNixHacks ? false, | ||
version ? "7.3.1", |
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.
7.3.2 is also available now
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.
And 7.4.0
Sorry for forgetting about this. The ofborg failure looks like a build sandbox thing, maybe. The default setting on macOS (used by Hydra) is to leave it disabled, but ofborg uses the sandbox since it‘s building stuff from random users on the internet. |
BTW, this isn‘t directly related to this PR but I thought I should note it down for others in case I don‘t get around to handling it: Bazel 5 is leaving security support two months into 24.11, and only one package uses it, so it should probably get dropped before release. |
Ah it looks like the
which appears to be related to this open issue that we have fixed with a patch. Unfortunately, it looks like we can't build Bazel 7 on darwin with sandboxing turned on if we want to use this bootstrapping method. I see a couple immediate options:
There may be some better/more nuanced options as well. |
Don't we already "fork" bazel by applying our hermeticity patches? Sounds more like an advertising problem? The issue was reported in 2023 and hasn't gained much traction since then. Assuming the patch is just to disable the functionality to not sleep while a build is running, it seems worthwhile to get bazel 7.3 in Nix? I wonder if we could also modify that code to simply make it "best effort" as in, if the power API calls fail, just continue on and hope for the best? |
Also, I tried building your branch on OSX and I got this:
After making this change:
It built on OSX for me. Testing on an M1 Mac |
Yes, the problem is that, in order to use
Thank you, I will add that in! |
--tool_java_runtime_version=local_jdk_21 \ | ||
--java_runtime_version=local_jdk_21 \ |
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.
--tool_java_runtime_version=local_jdk_21 \ | |
--java_runtime_version=local_jdk_21 \ | |
--tool_java_runtime_version=remotejdk_21 \ | |
--java_runtime_version=remotejdk_21 \ |
It seems to not affect the vendor
results, but might be cleaner to specify a hermetic toolchain rather than host toolchain?
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.
though there's more use of local_jdk and patching related to that, so maybe local_jdk fits better
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.
Given that this is using the bootstrap bazel, I agree that your suggestion might be preferable, but isn't it using a hermetic toolchain we bring into the env instead?
(cd bazel_src; ${bazelForDeps}/bin/bazel --server_javabase=${runJdk} mod deps --curses=no; | ||
${bazelForDeps}/bin/bazel --server_javabase=${runJdk} vendor src:bazel_nojdk \ |
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.
(cd bazel_src; ${bazelForDeps}/bin/bazel --server_javabase=${runJdk} mod deps --curses=no; | |
${bazelForDeps}/bin/bazel --server_javabase=${runJdk} vendor src:bazel_nojdk \ | |
(cd bazel_src; ${bazelForDeps}/bin/bazel --batch --server_javabase=${runJdk} mod deps --curses=no; | |
${bazelForDeps}/bin/bazel --batch --server_javabase=${runJdk} vendor src:bazel_nojdk \ |
Noticed that without fhs wrapping build would fail on bazel daemonizing itself. Daemon mode shouldn't be required for this vendoring target, also in some cases it's easier to capture logs in batch mode
# A FOD that vendors the Bazel dependencies using Bazel's new vendor mode. | ||
# See https://bazel.build/versions/7.3.0/external/vendor for details. | ||
# Note that it may be possible to vendor less than the full set of deps in | ||
# the future, as this is approximately 16GB. |
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.
# the future, as this is approximately 16GB. | |
# the future, as this is approximately 0.9GB. |
For me it amounts to 840-870MB 🤔
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.
At the time that was written, it was vendoring the entire build rather than just the required targets.
# the future, as this is approximately 16GB. | ||
bazelDeps = | ||
let | ||
bazelForDeps = if stdenv.hostPlatform.isDarwin then bazelBootstrap else bazelFhs; |
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.
bazelForDeps = if stdenv.hostPlatform.isDarwin then bazelBootstrap else bazelFhs; | |
bazelForDeps = bazelBootstrap; |
with --batch
it seems that bazelBootstrap
works fine, not sure how exactly was daemon mode failing without the wrapper.
But also wrapper doesn't add any extra dependencies and looks like a no-op, at the very least it'd be nice to add a comment on why add FHS wrapper, and why not on MacOS.
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'm happy to get rid of the explicit FHS env if we don't need it, especially since we can't use it on darwin either way. Not sure if we should make this change right before it gets merged, though?
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 didn't try to run it on ofborg or hydra so maybe less time risk in merging current version and look into simplifying later
For some reason I'm hitting
on |
else if stdenv.hostPlatform.system == "aarch64-darwin" then | ||
"sha256-E6j31Sl+aGs6+Xdx+c0Xi6ryfYZ/ms5/HzIyc3QpMHY=" | ||
else | ||
# x86_64-darwin |
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.
technically it may cover other platforms too, like riscv or something
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.
That is not a supported platform. I would be happy to take a vendor sha for i686, but since I think it is unlikely that people are waiting to be able to run Bazel >= 7.3 via Nix on a Raspberry Pi, I don't want to gate merging this PR, which has been outstanding for months, on its inclusion.
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 I meant is it's ok to only support most popular platforms, but there are more direct ways to express it:
- here instead of "else # assume x86_64-darwin" do "else if ...darwin case... else fail"
- and/or platforms meta attribute https://ryantm.github.io/nixpkgs/stdenv/meta/#var-meta-platforms
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.
Ah I understand, you're right, I can make that change.
} | ||
else | ||
fetchurl { | ||
# stdenv.hostPlatform.system == "aarch64-darwin" |
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.
also here it assumes there's only 4 possible platform combinations
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.
See my comment above.
extra info is that with $ cntr exec ls /
build dev etc nix proc tmp var there seems to be no Shouldn't be a blocker I guess, if it works on Hydra let it work, if not maybe add aarch64-linux to "badPlatforms" for now |
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.
Personally don't see why not merge it even in current shape and bump&improve further later
ZHF is also starting so any regressions might be good to address during that campaign
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.
LGTM.
Bootstrapping from a binary tarball seems more reliable than everything we tried before.
Not updating tests is okay. They are cumbersome to update, and do not really work anyway. What we need is a maintained toy/example/test project that is nix/NixOS compatible. Not aware of any right now.
I plan to merge this on Thursday unless there is some hard blocker discovered in the meantime.
@layus might sound odd, but I wonder if we could use some examples/tests from rules_nixpkgs? Some of the unresolved suggestions are simple/straightforward. Should I go ahead and make these changes ahead of merge? |
Will this still be merged on Thursday? |
@flurie I am just trying to be helpful by providing the ability to merge, and a reminder that it does not need to be perfect to be useful. I will merge as-is, but changes in another PR are obviously welcome. |
Can we maybe also bump it to 7.4? Update of the version seems not to cause problems from a first test. |
#355457 waiting for the build |
Description of changes
Bazel 7.1.2 -> 7.3.1
Things done
Walking through this in some detail:
bazel vendor
for a fixed-output derivation. Eliminate any parts that are not reproducible but do not affect the output.VENDOR.bazel
. To enable offline builds, we need to set--repository_disable_download
, not--nofetch
. Also, for some reason py_binary rules were building python zips, so I had to add--nobuild_python_zip
.More serious gaps:
bazel vendor
performs when you aren't as all-in on bzlmod as Bazel's own build.bazel vendor
is hard to use as a standard deps fetch more generally, should we consider keeping Bazel 7.3 as a separate package for now and moving forward with replacements for things that can use it? That's the only reason I have kept them separate for now.nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.