Overhaul how VERSION.TXT is generated #21146
Conversation
|
+@jwnimmer-tri for feature review, please In theory, at least some experimental jobs are supposed to upload artifacts? I believe anything generated by CI will be unaffected due to the current behavior of replacing the Drake-generated |
|
BTW, since I believe it isn't easy to hang release notes off of drake-ci PRs, we possibly want to use this as the signpost that some artifact names will be changing? I'm not sure if that's a "fix" or "breaking change" though. |
|
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-bazel-experimental-packaging please |
There was a problem hiding this comment.
I'm not sure if we should land this without fixing more in the same pass?
It looks to me like the VERSION.TXT inside the whl file is better (per the new format logic here), but the whl filename now no longer matches this new version number.
Also the packaging build isn't affected it seems, because CI is overwriting the file?
I guess this improves the VERSION.TXT of local builds, but the more important thing is packaging and wheel builds, which seem either unchanged or worse as of this PR.
Is it practical to develop all the drake and drake-ci changes we need at once, run test builds with those, and then merge everything in its final form? It's hard for me to review this halfway step as-is, I think.
Reviewed 3 of 3 files at r1.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
mwoehlke-kitware
force-pushed
the
improve-version-generation
branch
from
6827f07
to
88d0401
Compare
|
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-wheel-experimental-release please macOS wheels weren't picking up the 'external' version; should be fixed now.
Correct. Currently (i.e. without this PR), the locally-generated |
|
See also RobotLocomotion/drake-ci#270. |
|
I would say this looks to me moving in generally the right direction, from a requirements point of view, so that's all you need from me. For feature review of the code, please find someone on the Kitware side to check that (1) this is a step in the right direction and (2) does not introduce any regressions. Then I can serve as platform reviewer, later on. |
|
-@jwnimmer-tri +@BetsyMcPhail for feature review, please. |
There was a problem hiding this comment.
I launched a set of jobs with this and the drake-ci change:
https://drake-jenkins.csail.mit.edu/view/Packaging/job/mac-arm-ventura-unprovisioned-clang-bazel-experimental-packaging/5/
https://drake-jenkins.csail.mit.edu/view/Wheel/job/mac-arm-sonoma-unprovisioned-clang-wheel-experimental-release/2/
https://drake-jenkins.csail.mit.edu/view/Packaging/job/linux-jammy-unprovisioned-gcc-bazel-experimental-packaging/17/
https://drake-jenkins.csail.mit.edu/view/Wheel/job/linux-jammy-unprovisioned-gcc-wheel-experimental-release/8/
Reviewed all commit messages.
Reviewable status: LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
There was a problem hiding this comment.
The Jammy packaging build is failing with the following error:
\[3:33:29 PM\] File "/media/ephemeral0/ubuntu/workspace/linux-jammy-unprovisioned-gcc-bazel-experimental-packaging/\_bazel\_ubuntu/be1a85e71d3e0420409c5186aea1d927/execroot/drake/bazel-out/k8-opt/bin/tools/release\_engineering/repack\_deb.runfiles/drake/tools/release\_engineering/repack\_deb.py", line 184, in <module>
\[3:33:29 PM\] main()
\[3:33:29 PM\] File "/media/ephemeral0/ubuntu/workspace/linux-jammy-unprovisioned-gcc-bazel-experimental-packaging/\_bazel\_ubuntu/be1a85e71d3e0420409c5186aea1d927/execroot/drake/bazel-out/k8-opt/bin/tools/release\_engineering/repack\_deb.runfiles/drake/tools/release\_engineering/repack\_deb.py", line 180, in main
\[3:33:29 PM\] \_run(args)
\[3:33:29 PM\] File "/media/ephemeral0/ubuntu/workspace/linux-jammy-unprovisioned-gcc-bazel-experimental-packaging/\_bazel\_ubuntu/be1a85e71d3e0420409c5186aea1d927/execroot/drake/bazel-out/k8-opt/bin/tools/release\_engineering/repack\_deb.runfiles/drake/tools/release\_engineering/repack\_deb.py", line 54, in \_run
\[3:33:29 PM\] version = archive.getmember('drake/share/doc/drake/VERSION.TXT')
\[3:33:29 PM\] File "/usr/lib/python3.10/tarfile.py", line 1978, in getmember
\[3:33:29 PM\] raise KeyError("filename %r not found" % name)
\[3:33:29 PM\] KeyError: "filename 'drake/share/doc/drake/VERSION.TXT' not found"
Reviewable status: 1 unresolved discussion, LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
CMakeLists.txt line 581 at r2 (raw file):
"-DINPUT_FILE=${PROJECT_SOURCE_DIR}/tools/install/libdrake/VERSION.TXT.in" "-DOUTPUT_FILE=${PROJECT_BINARY_DIR}/VERSION.TXT" -P "${PROJECT_SOURCE_DIR}/tools/install/libdrake/generate_version.cmake"
From the Mac experimental packaging job https://drake-jenkins.csail.mit.edu/view/Packaging/job/mac-arm-ventura-unprovisioned-clang-bazel-experimental-packaging/5/consoleFull, I downloaded https://drake-packages.csail.mit.edu/drake/experimental/drake-0.0.20240326.151331%2Bgit6238c107-mac-arm64.tar.gz. I expected to see VERSION.TXT in drake/share/doc/drake/ but the only file there was LICENSE.TXT
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
CMakeLists.txt line 581 at r2 (raw file):
Previously, BetsyMcPhail (Betsy McPhail) wrote…
From the Mac experimental packaging job https://drake-jenkins.csail.mit.edu/view/Packaging/job/mac-arm-ventura-unprovisioned-clang-bazel-experimental-packaging/5/consoleFull, I downloaded https://drake-packages.csail.mit.edu/drake/experimental/drake-0.0.20240326.151331%2Bgit6238c107-mac-arm64.tar.gz. I expected to see
VERSION.TXTindrake/share/doc/drake/but the only file there wasLICENSE.TXT
This (and also the Jammy failure) is a problem with the CI changes, not with this PR. Specifically, the Bazel packaging jobs don't use the CMake front-end and so are not able to get VERSION.TXT from there. We need to switch packaging jobs over to using CMake, but this should land before we do that, so we can use the VERSION.TXT from Drake's build. (Right now, CI is creating its own VERSION.TXT. We want CI to stop doing that.)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
mwoehlke-kitware
force-pushed
the
improve-version-generation
branch
from
88d0401
to
49b3512
Compare
|
@drake-jenkins-bot mac-arm-ventura-unprovisioned-clang-wheel-experimental-release please |
|
@drake-jenkins-bot linux-arm-jammy-unprovisioned-gcc-wheel-experimental-release please |
1 similar comment
|
@drake-jenkins-bot linux-arm-jammy-unprovisioned-gcc-wheel-experimental-release please |
|
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-wheel-experimental-release please |
|
D'oh. Ignore the ARM failures, no surprise that isn't working... |
|
At least in https://drake-jenkins.csail.mit.edu/job/linux-jammy-unprovisioned-gcc-wheel-experimental-release/13, it looks like the problem with the wheels reporting "HEAD" as the git SHA appears to be resolved. |
There was a problem hiding this comment.
I confirmed that both the linux and mac wheels have the expected VERSION.txt
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
There was a problem hiding this comment.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware)
There was a problem hiding this comment.
+@jwnimmer-tri for platform review please
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)
CMakeLists.txt line 578 at r3 (raw file):
COMMAND "${CMAKE_COMMAND}" "-DGIT_DIR=${GIT_DIR}" "-DGIT_EXECUTABLE=${GIT_EXECUTABLE}"
BTW If git is not installed and DRAKE_GIT_SHA is not set in the environment, does the build still succeed?
It seems like yes, but I don't know CMake well enough to be sure just by reading the code. Can you confirm for me?
tools/install/libdrake/generate_version.cmake line 4 at r3 (raw file):
set(BUILD_IDENTIFIER unknown) if(DEFINED ENV{DRAKE_GIT_SHA})
Using environment variables in the wheel builder code to pass settings from the main program down to lower-level helper scripts is perfectly fine. That's exclusively Drake-internal code, so has a fairly narrow scope.
I am less comfortable having this user-facing CMakeLists scraping environment variables to change its behavior, especially ones like DRAKE_VERSION that could easily be used in someone's CI system to specify which version of Drake they want to use as an external project.
Is it tractable to change the force mechanism here to use CMake variables instead of environment variables? (Presumably they would be marked as "advanced".) That seems nicer for CI drivers anyway, which are already speaking CMake variables natively.
Anyway, whether they are environment vars or cmake vars, either way I think they need a different name here. Something that indicates their "keep away, CI only" advanced nature, maybe DRAKE_FORCE_GIT_SHA and DRAKE_FORCE_VERSION or even DRAKE_CI_FORCE_GIT_SHA, etc.
|
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-packaging please |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), missing label for release notes
CMakeLists.txt line 578 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW If
gitis not installed andDRAKE_GIT_SHAis not set in the environment, does the build still succeed?It seems like yes, but I don't know CMake well enough to be sure just by reading the code. Can you confirm for me?
It should; if if(GIT_EXECUTABLE AND EXISTS "${GIT_DIR}") is not true, generate_version.cmake won't try to invoke git and BUILD_IDENTIFIER will remain set to unknown.
mwoehlke-kitware
force-pushed
the
improve-version-generation
branch
from
49b3512
to
89db496
Compare
|
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-wheel-experimental-release please |
Generate VERSION.TXT at build time rather than configure time. Since configuring only needs to happen infrequently, it was possible that the contents would be significantly out of date if a user configures, then after some time, fetches the latest changes and builds again. By moving generation to build time, we ensure that the time stamp (which includes a time and not just a day) is always maximally correct, and likewise that the git SHA is correct. Additionally, rather than just a time stamp, the first of the two values in VERSION.TXT now defaults to an actual version identifier of the usual form for experimental builds (i.e. 0.0.YYYYMMDD.HHMMSS+git<sha8>). This can be overridden by undocumented CMake variables, which are used by wheel and CI builds to inject "real" version numbers. (The git SHA can be similarly overridden, which is needed for Linux wheel builds as the Docker build environment has only the raw sources and is therefore unable to obtain the git SHA on its own.) Finally, tweak wheel builds to make use of these new mechanisms.
mwoehlke-kitware
force-pushed
the
improve-version-generation
branch
from
89db496
to
a7f7bf8
Compare
|
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-wheel-experimental-release please |
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), missing label for release notes (waiting on @mwoehlke-kitware)
tools/install/libdrake/generate_version.cmake line 4 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Using environment variables in the wheel builder code to pass settings from the main program down to lower-level helper scripts is perfectly fine. That's exclusively Drake-internal code, so has a fairly narrow scope.
I am less comfortable having this user-facing CMakeLists scraping environment variables to change its behavior, especially ones like
DRAKE_VERSIONthat could easily be used in someone's CI system to specify which version of Drake they want to use as an external project.Is it tractable to change the force mechanism here to use CMake variables instead of environment variables? (Presumably they would be marked as "advanced".) That seems nicer for CI drivers anyway, which are already speaking CMake variables natively.
Anyway, whether they are environment vars or cmake vars, either way I think they need a different name here. Something that indicates their "keep away, CI only" advanced nature, maybe
DRAKE_FORCE_GIT_SHAandDRAKE_FORCE_VERSIONor evenDRAKE_CI_FORCE_GIT_SHA, etc.
Done.
jwnimmer-tri
added
the
release notes: fix
There was a problem hiding this comment.
I'll say +(release notes: fix) so we remember to mention the new VERSION.TXT contents in the notes.
Reviewed 4 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion
a discussion (no related file):
Working
I'm going to peek at the experimental packages and make sure they look okay.
|
@drake-jenkins-bot linux-jammy-unprovisioned-gcc-bazel-experimental-packaging please |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
I'm going to peek at the experimental packages and make sure they look okay.
Wheels are good.
Just waiting on the packaging build now.
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),BetsyMcPhail
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Wheels are good.
Just waiting on the packaging build now.
Packaging build looks good.
Generate VERSION.TXT at build time rather than configure time. Since configuring only needs to happen infrequently, it was possible that the contents would be significantly out of date if a user configures, then after some time, fetches the latest changes and builds again. By moving generation to build time, we ensure that the time stamp (which includes a time and not just a day) is always maximally correct, and likewise that the git SHA is correct.
Additionally, rather than just a time stamp, the first of the two values in VERSION.TXT now defaults to an actual version identifier of the usual form for experimental builds (i.e. 0.0.YYYYMMDD.HHMMSS+git). This can be overridden by undocumented CMake variables, which are used by wheel and CI builds to inject "real" version numbers. (The git SHA can be similarly overridden, which is needed for Linux wheel builds as the Docker build environment has only the raw sources and is therefore unable to obtain the git SHA on its own.)
Finally, tweak wheel builds to make use of these new mechanisms.
Towards #18145.
This change is