[cmake] Use Bazelisk to download Bazel #20064
Conversation
jwnimmer-tri
added
the
release notes: fix
jwnimmer-tri
force-pushed
the
bazelisk
branch
2 times, most recently
from
f9d296e to
d1e3077
Compare
jwnimmer-tri
added
priority: low
status: commits are properly curated
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-cmake-experimental-release please |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
+@svenevs and/or +@mwoehlke-kitware for feature review, please. Feel free to unassign the other if you think your own review will be sufficient. |
|
@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-release please. running mac wheel while testing locally |
There was a problem hiding this comment.
Reviewed 1 of 6 files at r5.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
find_program(Bazel_EXECUTABLE NAMES bazel bazelisk bazelisk.py PATHS "${PROJECT_SOURCE_DIR}/third_party/com_github_bazelbuild_bazelisk"
Working, I'm having a hard time understanding our desired outcome here. Just using a single thread here, at the very least we probably want to add NO_DEFAULT_PATHS to CMake's find_program and probably every other NO_*_PATH variable.
When looking at how things are being done differently on macOS vs linux in the install_prereqs_user_environment.sh, I don't understand why we're keeping the split. I was under the impression that if we're going to vendor com_github_bazelbuild_bazelisk, then we want our setup/ scripts to stop installing bazel at all (on macOS we are still installing bazelisk from brew, and on linux we still download it if it is not already there).
I believe the impact here is that what happens in CMake and what happens in bazel are different now. Are we not trying to remove the stuff from setup/ using this?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
Working, I'm having a hard time understanding our desired outcome here. Just using a single thread here, at the very least we probably want to add
NO_DEFAULT_PATHSto CMake'sfind_programand probably every otherNO_*_PATHvariable.When looking at how things are being done differently on macOS vs linux in the
install_prereqs_user_environment.sh, I don't understand why we're keeping the split. I was under the impression that if we're going to vendorcom_github_bazelbuild_bazelisk, then we want oursetup/scripts to stop installing bazel at all (on macOS we are still installingbazeliskfrombrew, and on linux we still download it if it is not already there).I believe the impact here is that what happens in CMake and what happens in bazel are different now. Are we not trying to remove the stuff from
setup/using this?
If I understand the docs correctly, NO_DEFAULT_PATHS is the superset of NO_*_PATH.
Do we want that? It's unclear; as written, we'll still use bazel if it exists in the user's environment. I was (also?) under the impression the idea was to always use bazelisk, but maybe that impression is wrong?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
... at the very least we probably want to add
NO_DEFAULT_PATHS
What would be the upside of that? If the user has a proper bazel already installed on a default path, I think we should use it without any extra downloads. Our vendored bazelisk is intended to be a fallback of last resort.
The thing I didn't implement here, but considered as future work, would be to keep searching the paths (in a proper order) until a suitable version is found, in case some earlier hits were too old. My thinking was to defer that until necessary, since install_prereqs.sh still defaults to installing a proper bazel or bazelisk natively.
I don't understand why we're keeping the split.
Our vendored python bazelisk is neutered. The native homebrew version has a lot more features, and is better for Drake developers to use.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
... at the very least we probably want to add
NO_DEFAULT_PATHSWhat would be the upside of that? If the user has a proper bazel already installed on a default path, I think we should use it without any extra downloads. Our vendored bazelisk is intended to be a fallback of last resort.
The thing I didn't implement here, but considered as future work, would be to keep searching the paths (in a proper order) until a suitable version is found, in case some earlier hits were too old. My thinking was to defer that until necessary, since
install_prereqs.shstill defaults to installing a proper bazel or bazelisk natively.I don't understand why we're keeping the split.
Our vendored python bazelisk is neutered. The native homebrew version has a lot more features, and is better for Drake developers to use.
Or in other words...
If a Drake developer or user keeps doing the default "install prereqs with bazel" setup, the changes to this file are a no-op.
If a linux user does "install prereqs without bazel" (and does not have any bazel-like things installed anywhere, e.g., in a docker container) then this will fall back to the vendored bazelisk. This allows people building using CMake in a container to depend only on stock Ubuntu debs.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 6 of 12 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 5 of 6 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Or in other words...
If a Drake developer or user keeps doing the default "install prereqs with bazel" setup, the changes to this file are a no-op.
If a linux user does "install prereqs without bazel" (and does not have any bazel-like things installed anywhere, e.g., in a docker container) then this will fall back to the vendored bazelisk. This allows people building using CMake in a container to depend only on stock Ubuntu debs.
Ok, I don't think this does that correctly. Before looking at implementing, if somebody (a) has bazel installed and (b) it is too old of a version, should the build fail? Or should it get the one we need?
Making it fail makes life a little easier, but the idea is in the CMakeLists.txt we probably want to do something a little more differently:
# The version passed to find_package(Bazel) should match the
# minimum_bazel_version value in the call to versions.check() in WORKSPACE.
set(MINIMUM_BAZEL_VERSION 4.0)
find_package(Bazel ${MINIMUM_BAZEL_VERSION} MODULE)
if(NOT Bazel_FOUND)
# manually search our vendored third party and run `bazelisk version`?
# do not put this logic in the find module, makes it more difficult for no reason
# e.g., I don't want to find_package(Bazel) here with a new Bazel_ROOT
# and different names to search for (bazelisk and bazelisk.py)
endif()
Like, I think we want it to "find the first bazel" (user is responsible for this, e.g., via PATH). If not found, we can either error, or get the right one. If their system version is too old, we have a similar choice.
tools/wheel/image/build-drake.sh line 12 at r5 (raw file):
git apply < /image/pip-drake.patch export BAZELISK_HOME=/var/cache/bazel/bazelisk
working, this path concerns me, waiting for the mac monterey queue to run the wheel job but if it takes too long I will test locally
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
If somebody (a) has bazel installed and (b) it is too old of a version, should the build fail? Or should it get the one we need?
All else being equal, fetching the one we need would be nice. However, on master today the build would fail, so sticking with "fail" for now is OK.
I don't think this does that correctly.
Could you describe the conditions where something goes wrong?
In any case, I need to turn some of the context in this discussion into comments in the code, but I'm holding off on that until we're on the same page.
tools/wheel/image/build-drake.sh line 12 at r5 (raw file):
Previously, svenevs (Stephen McDowell) wrote…
working, this path concerns me, waiting for the mac monterey queue to run the wheel job but if it takes too long I will test locally
This is the path where downloads of the versioned bazel get dumped. It seemed nice to keep them in the cache instead of always re-downloading them.
(I should add a comment about this.)
|
Okay, I guess I'd missed that. I wonder if this isn't over-thinking things. I suspect it makes sense to accept |
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
On homebrew, brew bazelisk provides both bazel and bazelisk on the default path (and they are both symlinks to the bottled bazelisk binary).
Maybe we should just make the
find_packagenot required ...
Now that you mention it, that does sound a lot simpler. I'll try that out locally.
|
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Right; I think our current thoughts are:
Stephen also pointed out that if the module finds Bazelisk, the |
There was a problem hiding this comment.
Reviewed 6 of 12 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 4 of 6 files at r5.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
-- commits line 9 at r5:
FYI, I take it this is the list of people that authored bazelisk.py? (Do we normally do this when importing sources, vs. just having authorship information alongside somewhere?)
tools/workspace/bazelisk/test/lint_test.py line 9 at r5 (raw file):
$ cp bazel-drake/external/bazelisk/LICENSE \ bazel-drake/external/bazelisk/bazelisk.py \ third_party/com_github_bazelbuild_bazelisk/
cp -t¹ won't accidentally overwrite src2 with src1... (One might add -i also, but that might be excessive.)
(¹ ...although -t is GNU-only; is it okay to specify doing this on Ubuntu?)
Suggestion:
$ cp -t third_party/com_github_bazelbuild_bazelisk/ \
bazel-drake/external/bazelisk/LICENSE \
bazel-drake/external/bazelisk/bazelisk.py
jwnimmer-tri
force-pushed
the
bazelisk
branch
2 times, most recently
from
bfa81ce to
084a19b
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees svenevs,mwoehlke-kitware, needs platform reviewer assigned
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
FYI, I take it this is the list of people that authored bazelisk.py? (Do we normally do this when importing sources, vs. just having authorship information alongside somewhere?)
I got carried away. Removed.
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Right; I think our current thoughts are:
- Don't add
PATHSto the find module.NAMES bazeliskis probably good; not sure ifbazelisk.pystill makes sense.find_package(Bazel)is notREQUIREDif(NOT Bazel_FOUND)then setBazel_EXECUTABLEto vendored Bazelisk. Maybe warn that we're doing so.Stephen also pointed out that if the module finds Bazelisk, the
execute_processis going to download Bazel. We might want to try to detect first ifBazel_EXECUTABLEis Bazelisk and skip the version check if so.
Makes sense, thanks. I took a stab at it.
tools/wheel/image/build-drake.sh line 12 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This is the path where downloads of the versioned
bazelget dumped. It seemed nice to keep them in the cache instead of always re-downloading them.(I should add a comment about this.)
Commented.
tools/workspace/bazelisk/test/lint_test.py line 9 at r5 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
cp -t¹ won't accidentally overwrite src2 with src1... (One might add-ialso, but that might be excessive.)(¹ ...although
-tis GNU-only; is it okay to specify doing this on Ubuntu?)
It's probably only going to be run on Ubuntu, but anyway I don't think the mistake is very likely or recovery from it very difficult. If we find the instructions are too difficult as-is, the plan would be to provide a program to do it (e.g. an --update argparse flag to this test).
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-cmake-experimental-release please @drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-cmake-experimental-release please @drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-release please |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee svenevs, needs platform reviewer assigned, when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I got carried away. Removed.
👍
cmake/modules/FindBazel.cmake line 6 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Makes sense, thanks. I took a stab at it.
👍
tools/workspace/bazelisk/test/lint_test.py line 9 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It's probably only going to be run on Ubuntu, but anyway I don't think the mistake is very likely or recovery from it very difficult. If we find the instructions are too difficult as-is, the plan would be to provide a program to do it (e.g. an
--updateargparse flag to this test).
🤷
There was a problem hiding this comment.
+@rpoyner-tri for platform review, please.
We still have some feature comments from @svenevs pending, but I'm hoping this is close enough to final form that platform review now will be OK.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees svenevs,rpoyner-tri(platform), when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 4 of 4 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
tools/wheel/image/build-drake.sh line 12 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Commented.
done, I misunderstood what the impact of this path was
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 6 of 12 files at r2, 1 of 1 files at r3, 3 of 6 files at r5, 4 of 4 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: when planning a "properly curated" merge commit the PR must always be rebased onto latest master (waiting on @jwnimmer-tri)
This is a copy as of bazelbuild/bazelisk#494. In a future commit, we'll add infrastructure to keep our copy in sync.
Closes #19923.
See also bazelbuild/bazelisk#494.
This change is