Upgrade Bazel to 6.0.0 #18545
Upgrade Bazel to 6.0.0 #18545
Conversation
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-release please |
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-release please |
|
@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please |
|
@drake-jenkins-bot linux-focal-unprovisioned-clang-bazel-experimental-leak-sanitizer please |
|
The mac tests are currently failing due to an issue with dReal, first mentioned here: #18246 (comment). Since dReal is scheduled to be removed in less than a month as per #18156, would it make sense to put this PR on hold until dReal has been removed? Removing dReal should fix the test which is currently failing. |
|
On first glance, it looks easy to patch: --- tools/dreal.bzl.orig
+++ tools/dreal.bzl
@@ -65,7 +65,6 @@
return select({
"//tools:gcc_build": GCC_FLAGS + extra_gcc_flags + rule_copts,
"//tools:clang_build": CLANG_FLAGS + rule_copts,
- "//tools:apple": CLANG_FLAGS + rule_copts,
"//conditions:default": CXX_FLAGS + rule_copts,
})
|
|
@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please |
|
I edited the PR overview to say |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @williamjallen)
tools/workspace/dreal/patches/pull18545.patch line 1 at r3 (raw file):
See https://github.com/RobotLocomotion/drake/pull/18545 for details
Patch files are not automatically applied. Each one needs to be listed out individually:
drake/tools/workspace/dreal/repository.bzl
Lines 17 to 22 in 30b97d8
|
@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please |
|
@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please |
|
@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please |
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @williamjallen)
williamjallen
added
component: build system
jwnimmer-tri
added
release notes: fix
|
@drake-jenkins-bot mac-x86-monterey-clang-cmake-experimental-release please @drake-jenkins-bot linux-focal-unprovisioned-gcc-cmake-experimental-debug please @drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-debug please @drake-jenkins-bot linux-focal-unprovisioned-clang-bazel-experimental-debug please @drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-documentation please @drake-jenkins-bot linux-focal-gcc-bazel-weekly-everything-coverage please |
There was a problem hiding this comment.
Once CI finishes running successfully, we can assign platform review.
Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @williamjallen)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Summarizing the slack discussions I had with @williamjallen in the
#buildchannel:We were able to chase down bazelbuild/bazel#15635 as the root cause of the macOS Debug bloat.
In the overall macOS Debug build, we observed a 6.3% uptick in disk use. Looking at a dump of the files sizes in the outputRoot, the only substantial change disk between 5.3 and 6.0 was to executable binaries, mostly in C++ unit tests. Those binaries were ~9% larger on average.
Comparing the linker command for 5.3 vs 6.0, the only difference was
-Xlinker -no_deduplicate. Searching that in github.com/bazelbuild located the problematic pull request, which landed in 6.0This PR now disables that feature, so hopefully the in-progress CI Debug build will succeed, and we can close this thread.
Done.
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Before merging this PR, we need to re-run the full suite of configurations. See the [https://github.com//pull/18545#issuecomment-1377463897](two comments here) for the list.
Done.
tools/skylark/drake_cc.bzl line 807 at r9 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit As it stands, someone in the future who is reading this code will probably not understand what's going on here. We should have a comment here that explains / justifies the thought process surrounding this flag.
Done.
This comment was marked as outdated.
This comment was marked as outdated.
|
+@SeanCurtis-TRI for platform review per schedule, please. |
There was a problem hiding this comment.
There are a couple small updates recalled by grep -ri bazel_version added.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI and @williamjallen)
a discussion (no related file):
One small location missed:
The wheel build does its own thing (via docker), after that is changed run the following build on this PR: https://drake-jenkins.csail.mit.edu/view/Wheel/job/linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release/
a discussion (no related file):
@jwnimmer-tri should this PR also fixup the inconsistency between
Line 88 in e201447
and
Line 22 in e201447
by setting them to 6?
a discussion (no related file):
Working, update this to 6.0.0 as well:
I'm running a local build to test the script still runs right now but there is no CI attached to that code.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI, @svenevs, and @williamjallen)
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
@jwnimmer-tri should this PR also fixup the inconsistency between
Line 88 in e201447
and
Line 22 in e201447
by setting them to 6?
It's not great that they are different, but they should definitely not say 6.0. We should only hard-fail the build if we know for sure that <6 will fail later on. Since the 5.3.1 version obviously still works, 6.0 here would be wrong.
If you like we can switch CMake to say 4.0 to match WORKSPACE, but that's the most we should do. (At least, not without evidence of a known failure mode of some prior version.)
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI and @williamjallen)
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
Working, update this to 6.0.0 as well:
I'm running a local build to test the script still runs right now but there is no CI attached to that code.
Builds as expected, updating the version number is safe. No new artifacts will be uploaded (there is no reason AFAICT).
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri, @SeanCurtis-TRI, and @williamjallen)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It's not great that they are different, but they should definitely not say
6.0. We should only hard-fail the build if we know for sure that <6 will fail later on. Since the 5.3.1 version obviously still works,6.0here would be wrong.If you like we can switch CMake to say 4.0 to match WORKSPACE, but that's the most we should do. (At least, not without evidence of a known failure mode of some prior version.)
OK, I'm going to leave this alone for this PR -- was not clear if we were supposed to have been updating this number as we bump bazel versions or not. Since it is not, I'll unify them to 4 in a separate PR really quick (we're hoping this PR's diff will model the next update without missing anything so that we can reference it in the future).
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI, @svenevs, and @williamjallen)
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
OK, I'm going to leave this alone for this PR -- was not clear if we were supposed to have been updating this number as we bump bazel versions or not. Since it is not, I'll unify them to 4 in a separate PR really quick (we're hoping this PR's diff will model the next update without missing anything so that we can reference it in the future).
Sounds fine.
(Note, however, that my post-merge plan here is to fix the wheel builds to reuse the bazel setup logic, instead of roll their own. There should only be two places we need to update (the dotfile for mac, and the setup script for ubuntu. So hopefully next time we won't need to touch the wheel builds.)
There was a problem hiding this comment.
Reviewed 4 of 4 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI, @svenevs, and @williamjallen)
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r10, 3 of 4 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Sounds fine.
(Note, however, that my post-merge plan here is to fix the wheel builds to reuse the bazel setup logic, instead of roll their own. There should only be two places we need to update (the dotfile for mac, and the setup script for ubuntu. So hopefully next time we won't need to touch the wheel builds.)
Done, ok that sounds much better. Thanks!
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
One small location missed:
The wheel build does its own thing (via docker), after that is changed run the following build on this PR: https://drake-jenkins.csail.mit.edu/view/Wheel/job/linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release/
Done
a discussion (no related file):
Previously, svenevs (Stephen McDowell) wrote…
Builds as expected, updating the version number is safe. No new artifacts will be uploaded (there is no reason AFAICT).
Done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r10, 3 of 4 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @williamjallen)
tools/skylark/drake_cc.bzl line 730 at r12 (raw file):
linkopts = linkopts, features = [ # We should deduplicate symbols while linking (for a ~6% reduction
BTW This comment seems a bit surprising; the text here says "should deduplicate", but the flag is -no_deduplicate. That seems inherently contradictory. What am I missing?
(Same below.)
tools/skylark/drake_cc.bzl line 731 at r12 (raw file):
features = [ # We should deduplicate symbols while linking (for a ~6% reduction # in disk use), to conserse space in CI; see #18545 for details.
nit s/conserse/conserve
(Same below.)
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI and @williamjallen)
tools/skylark/drake_cc.bzl line 730 at r12 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW This comment seems a bit surprising; the text here says "should deduplicate", but the flag is
-no_deduplicate. That seems inherently contradictory. What am I missing?(Same below.)
There is a negative sign before the "no", so we have a double negative.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @williamjallen)
tools/skylark/drake_cc.bzl line 730 at r12 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
There is a negative sign before the "no", so we have a double negative.
Riiiiiight. I misread that. If it had truly been a flag, it would've been --no_..., an apparent triple negative. :/ That's horrible UX.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @williamjallen)
tools/skylark/drake_cc.bzl line 730 at r12 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Riiiiiight. I misread that. If it had truly been a flag, it would've been
--no_..., an apparent triple negative. :/ That's horrible UX.
For more fun, some linker flags are single dash -foo to enable foo. In any case, the bottom line is that features is a bazel thing, so it's a list of toolchain feature strings to opt-in or opt-out of; they are not passed on any command-line as any kind of flag.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @williamjallen)
jwnimmer-tri
added
the
status: squashing now
Co-authored-by: William Allen <william.allen@kitware.com> Co-authored-by: Jeremy Nimmer <jeremy.nimmer@tri.global>
Co-authored-by: William Allen <william.allen@kitware.com> Co-authored-by: Jeremy Nimmer <jeremy.nimmer@tri.global>
Co-authored-by: William Allen <william.allen@kitware.com> Co-authored-by: Jeremy Nimmer <jeremy.nimmer@tri.global>
Co-authored-by: William Allen <william.allen@kitware.com> Co-authored-by: Jeremy Nimmer <jeremy.nimmer@tri.global>
This PR is currently in development and serves as a way to test changes on Jenkins during the upgrade to Bazel 6.0.0.
Closes #18508.
This change is