-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[build] Patch rules_cc to recognize cc as gcc #23738
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
4e942ef to
8441aec
Compare
|
@drake-jenkins-bot linux-jammy-gcc-cmake-experimental-release please |
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Working
Wheel builds fail with:
collect2: fatal error: cannot find 'ld'
But only the wheel? Is gcc trying to do something with a relative path, or do we need to append to PATH explicitly for the build somewhere for our symlink trick to work?
jwnimmer-tri
left a comment
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.
@jwnimmer-tri reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
tools/cc_toolchain/BUILD.bazel line 34 at r1 (raw file):
# warning spam during the build. # # These are intended for internal use only, not for users to configure.
typo
Suggestion:
This isCMakeLists.txt line 309 at r1 (raw file):
cmake_path(GET CMAKE_C_COMPILER FILENAME C_COMPILER_BASENAME) if (NOT C_COMPILER_BASENAME MATCHES "(gcc|clang)(-\\d+)?")
How is our CI coverage of this if-check?
Off the top of my head, nothing except DEE will be using cc here -- I think our CI always sets a more specific path, doesn't it?
If that's true, I wonder if nixing this if-check and always running the symlink code would be more robust?
tyler-yankee
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
How is our CI coverage of this if-check?
Off the top of my head, nothing except DEE will be using
cchere -- I think our CI always sets a more specific path, doesn't it?If that's true, I wonder if nixing this if-check and always running the symlink code would be more robust?
Yes, CI always sets gcc or clang based on the job name.
I provided the if-check for users' sake (if we don't need to do the symlinks, then don't try and risk something going wrong), but you have a fair point. It's also theoretically possible that getting rid of it solves the wheel build issue, though doubtful ... I haven't gotten a chance to dig further into that one locally yet.
jwnimmer-tri
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Yes, CI always sets gcc or clang based on the job name.
I provided the if-check for users' sake (if we don't need to do the symlinks, then don't try and risk something going wrong), but you have a fair point. It's also theoretically possible that getting rid of it solves the wheel build issue, though doubtful ... I haven't gotten a chance to dig further into that one locally yet.
Yup, I view it the same way. Not using a symlink at is possible risk reduction.
Another idea -- we could keep this code as-is with the if-check, but instead change drake-ci to set /usr/bin/cc as the compiler in at least one (or all) of the CMake GCC builds but keep /usr/bin/clang in the other(s).
tyler-yankee
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yup, I view it the same way. Not using a symlink at is possible risk reduction.
Another idea -- we could keep this code as-is with the if-check, but instead change
drake-cito set/usr/bin/ccas the compiler in at least one (or all) of the CMake GCC builds but keep/usr/bin/clangin the other(s).
I don't hate that idea. When we originally discovered this bug months back on DEE it had lingered because we didn't have CI coverage (we always set gcc/clang, and issue triggers when we don't).
Not a huge fan of picking out arbitrary jobs and maintaining custom logic. I guess if we did all the CMake GCC builds, we mostly still have coverage of actually taking the compiler you've specified, because of the Clang case?
jwnimmer-tri
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
I don't hate that idea. When we originally discovered this bug months back on DEE it had lingered because we didn't have CI coverage (we always set gcc/clang, and issue triggers when we don't).
Not a huge fan of picking out arbitrary jobs and maintaining custom logic. I guess if we did all the CMake GCC builds, we mostly still have coverage of actually taking the compiler you've specified, because of the Clang case?
Exactly.
tyler-yankee
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Exactly.
I suppose we could land this change ahead of time, but probably shouldn't because of the externals warning spam. I can prepare it alongside here and we can test+land in tandem?
tyler-yankee
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
I suppose we could land this change ahead of time, but probably shouldn't because of the externals warning spam. I can prepare it alongside here and we can test+land in tandem?
Wait a sec, let's back up.
CI runs bazel run --config=${COMPILER} //tools/cc_toolchain:print_compiler_config, where it determines COMPILER from the job name. (It does this under all configurations, not just CMake.)
Why can't we just have Drake's CMake run this when it detects a GCC or a Clang (*_COMPILER_ID), and set CC and CXX according to what it prints out? And forget about the whole symlink approach?
jwnimmer-tri
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Wait a sec, let's back up.
CI runs
bazel run --config=${COMPILER} //tools/cc_toolchain:print_compiler_config, where it determinesCOMPILERfrom the job name. (It does this under all configurations, not just CMake.)Why can't we just have Drake's CMake run this when it detects a GCC or a Clang (
*_COMPILER_ID), and set CC and CXX according to what it prints out? And forget about the whole symlink approach?
I don't follow.
Certainly we can't use --config=... because that's imbuing Bazel with Drake's very specific choices about exactly what should be run and how, as encoded in our rcfiles. Big picture, it's selecting one of our "developer profiles" that are tightly coupled with our supported platforms.
We could run //tools/cc_toolchain:print_compiler_config on its own, but how does that help anyway? It just echos back CC and CXX, which we already know in CMake land.
tyler-yankee
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I don't follow.
Certainly we can't use
--config=...because that's imbuing Bazel with Drake's very specific choices about exactly what should be run and how, as encoded in our rcfiles. Big picture, it's selecting one of our "developer profiles" that are tightly coupled with our supported platforms.We could run
//tools/cc_toolchain:print_compiler_configon its own, but how does that help anyway? It just echos back CC and CXX, which we already know in CMake land.
Oh, okay. I didn't realize the former, and yeah, it falls apart without --config because of the latter. Nevermind!
jwnimmer-tri
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 309 at r1 (raw file):
I can prepare it alongside here and we can test+land in tandem?
Sure. I'd also be OK with it as a second pass afterward.
tyler-yankee
left a comment
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.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Working
Wheel builds fail with:
collect2: fatal error: cannot find 'ld'But only the wheel? Is
gcctrying to do something with a relative path, or do we need to append toPATHexplicitly for the build somewhere for our symlink trick to work?
Okay, I can repro on Linux locally. My local Mac wheel build has gotten far enough at this point that we can conclude it doesn't have the issue. So it's some issue from within the Docker container, most likely.
8441aec to
d95b02f
Compare
tyler-yankee
left a comment
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.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Okay, I can repro on Linux locally. My local Mac wheel build has gotten far enough at this point that we can conclude it doesn't have the issue. So it's some issue from within the Docker container, most likely.
The best solution I can find here is hard-coding CMAKE_{C|CXX}_COMPILER, to bypass the new workaround. This doesn't seem entirely bad, because the prereqs explicitly list gcc / gcc-c++, so we know that's what they've been using all along (hidden under cc / c++).
I'm open to other ideas here.
jwnimmer-tri
left a comment
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.
@jwnimmer-tri reviewed all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
The best solution I can find here is hard-coding
CMAKE_{C|CXX}_COMPILER, to bypass the new workaround. This doesn't seem entirely bad, because the prereqs explicitly listgcc/gcc-c++, so we know that's what they've been using all along (hidden undercc/c++).I'm open to other ideas here.
The hard-coding doesn't bother me. What does is that apparently our CMakeLists.txt doesn't work under certain conditions (e.g., almalinux). We should get to the bottom of why that is/was.
d95b02f to
6173319
Compare
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The hard-coding doesn't bother me. What does is that apparently our CMakeLists.txt doesn't work under certain conditions (e.g., almalinux). We should get to the bottom of why that is/was.
Scratch notes:
- it successfully creates a link to
/usr/bin/gcc. - changing the name of the symlink to
foodoesn't change anything (so it's not somehow confusing the GCC executable with the local version) - making an additional symlink
ld -> /usr/bin/lddoesn't change anything (so GCC isn't looking for anldalongside it / in the same directory) /usr/binis onPATH
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Scratch notes:
- it successfully creates a link to
/usr/bin/gcc.- changing the name of the symlink to
foodoesn't change anything (so it's not somehow confusing the GCC executable with the local version)- making an additional symlink
ld -> /usr/bin/lddoesn't change anything (so GCC isn't looking for anldalongside it / in the same directory)/usr/binis onPATH
Ah. The third bullet point above was the right idea, but it needs to be ld.gold instead of ld, since Bazel by default uses -fuse-ld=gold.
I'll ponder what the best way to approach this programmatically is. CMake does provide CMAKE_{C|CXX}_COMPILER_LINKER, but it's not always guaranteed to be defined/provided, and this case under the Alma Linux Docker container it happens to not be.
jwnimmer-tri
left a comment
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.
@jwnimmer-tri reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Ah. The third bullet point above was the right idea, but it needs to be
ld.goldinstead ofld, since Bazel by default uses-fuse-ld=gold.I'll ponder what the best way to approach this programmatically is. CMake does provide
CMAKE_{C|CXX}_COMPILER_LINKER, but it's not always guaranteed to be defined/provided, and this case under the Alma Linux Docker container it happens to not be.
If GCC uses something like $(dirname $0) to find its linker, then the whole symlink idea is toast. There is no reasonable way to symlink all of the subprograms it wants to use at the right paths.
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
If GCC uses something like
$(dirname $0)to find its linker, then the whole symlink idea is toast. There is no reasonable way to symlink all of the subprograms it wants to use at the right paths.
Yeah, fair. We're back to iterating over readlink until we find something that looks like the right name, then?
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Yeah, fair. We're back to iterating over
readlinkuntil we find something that looks like the right name, then?
d'oh -- it turns out we just need binutils-gold added to the packages installed on the image, and don't need the linker symlink (this was intended to be my next experiment on Friday after proving the symlink worked, but ran out of time). Is this a reasonable expectation, to keep the symlink-the-compiler approach alive?
This whole mess happens because Bazel defaults to ld.gold, but that linker is no longer being supported upstream, and as such Fedora's decided to package it separately. I guess GCC has a fallback mechanism to finding it when binutils-gold isn't installed, and the current wheel builds are relying on that mechanism implicitly. But we know (at least for now, until a future Bazel change) that's what the wheel builds are using.
(Separate, but related question -- why did we decide to bring in mold but only under some configurations i.e. debug? Why the bifurcation?)
jwnimmer-tri
left a comment
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.
@jwnimmer-tri reviewed 1 of 1 files at r2.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
(Separate, but related question -- why did we decide to bring in
moldbut only under some configurations i.e. debug? Why the bifurcation?)
We try to stick with the defaults unless there is an acute reason to change. Having our suite of release builds use standard tools (and CI check it) means that users are more likely to succeed when they build Drake.
CMakeLists.txt line 298 at r3 (raw file):
... rules_cc only identifies the compiler based on the filename (rather than actually running it, as CMake does) ...
Actually, this isn't accurate. Bazel does run it:
I think the Bazel vs CMake distinction is that Bazel just runs the compiler and looks at what it printed, while CMake runs the compiler to build a program and then runs that program (which then uses the pre-processor to sniff the compiler).
In terms of Bazel not working, out only problem is that /usr/bin/cc --version doesn't print "gcc", it prints argv[0] (as noted by the comment in rules_cc).
(1) For starters, that means we don't need to symlink in the case of clang, only gcc.
(2) Maybe we can actually PR to rules_cc with a fix? Instead of gcc --version (which prints argv[0]) we could run gcc -v (which prints "gcc version ##.#").
(2b) Plausibly we could cherry-pick that patch file of the rules_cc PR into Drake in our MODULE.bazel and cmake/MODULE.bazel.in files so that we can land this fix prior to rules_cc accepting our patch and tagging a release.
If we prune out the clang parts here, then I'm also OK to land this as-is with the symlink, and leave the rules_cc to a follow-up.
CMakeLists.txt line 309 at r3 (raw file):
cmake_path(GET CMAKE_C_COMPILER FILENAME C_COMPILER_BASENAME) if (NOT C_COMPILER_BASENAME MATCHES "(gcc|clang)(-\\d+)?")
nit We need this regex to be anchored (^$). Is it anchored by default?
Also we should align with rules_cc precisely, so either ^gcc$ or ^gcc-.*$.
tools/wheel/image/build-drake.sh line 55 at r3 (raw file):
-DPython_EXECUTABLE=/usr/local/bin/python \ -DCMAKE_C_COMPILER=/usr/bin/gcc \ -DCMAKE_CXX_COMPILER=/usr/bin/g++
BTW With a fixed-up CMakeLists.txt and dependency updates, we can revert this part of the change now?
tyler-yankee
left a comment
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.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 298 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
... rules_cc only identifies the compiler based on the filename (rather than actually running it, as CMake does) ...
Actually, this isn't accurate. Bazel does run it:
I think the Bazel vs CMake distinction is that Bazel just runs the compiler and looks at what it printed, while CMake runs the compiler to build a program and then runs that program (which then uses the pre-processor to sniff the compiler).
In terms of Bazel not working, out only problem is that
/usr/bin/cc --versiondoesn't print "gcc", it printsargv[0](as noted by the comment in rules_cc).(1) For starters, that means we don't need to symlink in the case of clang, only gcc.
(2) Maybe we can actually PR to
rules_ccwith a fix? Instead ofgcc --version(which prints argv[0]) we could rungcc -v(which prints "gcc version ##.#").(2b) Plausibly we could cherry-pick that patch file of the rules_cc PR into Drake in our
MODULE.bazelandcmake/MODULE.bazel.infiles so that we can land this fix prior to rules_cc accepting our patch and tagging a release.If we prune out the clang parts here, then I'm also OK to land this as-is with the symlink, and leave the
rules_ccto a follow-up.
Thank you for digging that up (I swear I almost happened upon that file? apparently not)! Yes, then it seems much more straightforward to create a patch and ditch the symlinks altogether. I'll play with the implementation locally.
tools/wheel/image/build-drake.sh line 55 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW With a fixed-up CMakeLists.txt and dependency updates, we can revert this part of the change now?
yep, thanks (I've had this locally, just wanted to wait for dust to settle)
6173319 to
4a83492
Compare
tyler-yankee
left a comment
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'd like to ask around internally for folks with non-Ubuntu Linux and make sure the cc -v approach works where possible there as well.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
jwnimmer-tri
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
tools/workspace/rules_cc/patches/upstream/cc-as-gcc.patch line 16 at r4 (raw file):
- cc_stdout = repository_ctx.execute([cc, "--version"]).stdout - return cc_stdout.startswith("gcc ") or cc_stdout.startswith("gcc-") + return "gcc" in repository_ctx.execute([cc, "-v"]).stderr
This is probably too generous. If you look at clang -v output it still mentions GCC (albeit upper case).
We should at least look for "gcc version", possibly even looking only in the last line.
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
tools/workspace/rules_cc/patches/upstream/cc-as-gcc.patch line 16 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This is probably too generous. If you look at
clang -voutput it still mentions GCC (albeit upper case).We should at least look for "gcc version", possibly even looking only in the last line.
Thanks. I only checked cc -> clang for Apple Clang, should have also checked on Linux.
jwnimmer-tri
left a comment
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.
@jwnimmer-tri reviewed 5 of 6 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 293 at r4 (raw file):
set(BAZEL_REPO_ENV) if(NOT APPLE)
The if(NOT APPLE) guard around setting CC and CXX is still important / required. We want to let Bazel's XCode hunting logic do its default things, not override it with env vars.
4a83492 to
33d7b9b
Compare
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
CMakeLists.txt line 293 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The
if(NOT APPLE)guard around settingCCandCXXis still important / required. We want to let Bazel's XCode hunting logic do its default things, not override it with env vars.
Done.
tools/workspace/rules_cc/patches/upstream/cc-as-gcc.patch line 16 at r4 (raw file):
Previously, tyler-yankee (Tyler Yankee) wrote…
Thanks. I only checked
cc -> clangfor Apple Clang, should have also checked on Linux.
I've adapted the condition to be more strict.
jwnimmer-tri
left a comment
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 seems plausible now.
@jwnimmer-tri reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
a discussion (no related file):
Working
Testing with RobotLocomotion/drake-ci#363
tyler-yankee
left a comment
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.
@jwnimmer-tri Would you like an additional Kitware feature-review here (is yours serving as feature or platform)?
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
jwnimmer-tri
left a comment
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 can count as feature review of the Bazel changes, but please grab a Kitware helper to feature-review the CMake changes.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
tyler-yankee
left a comment
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.
+a:@mwoehlke-kitware for feature review on the CMake changes? Feel free to delegate as needed.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware
mwoehlke-kitware
left a comment
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 not clear what I'm supposed to be reviewing here. I see three CMakeLists.txt changes:
- Create symlink
${DRAKE_BAZEL_BUILD_DIR}/cc-as-gcc.patch. It isn't clear why this is needed, but that may be because I don't understand how this patch gets applied. This isn't exactly a "CMake" change. - Don't pass
--@drake//tools/cc_toolchain:compilerto macOS builds. This isn't really a "CMake" change. - Use
${DRAKE_BAZEL_BUILD_DIR}instead of repeating the literal name everywhere. These changes look fine.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware
|
I think we can count my review as "the logic is correct". What I'm looking for from @mwoehlke-kitware it whether the CMake code is spelled well / canonically, good variable names, etc.
@tyler-yankee let's add a comment in that case, to clarify. |
mwoehlke-kitware
left a comment
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.
Okay, in that case, the only changes of that nature are:
- The symlink. Yes, this could probably stand a comment.
${DRAKE_BAZEL_BUILD_DIR}. Looks fine to me.
The other change is pure deletion, so passes the "spelled well / canonically, good variable names, etc." by default.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware
tyler-yankee
left a comment
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.
Don't pass
--@drake//tools/cc_toolchain:compilerto macOS builds. This isn't really a "CMake" change.
(fwiw, (a) the crux of the change is to remove this flag altogether, and (b) the if(NOT APPLE) on CC and CXX is maintained from the current master - but @jwnimmer-tri's review covers this logic in any case)
let's add a comment in that case, to clarify.
Can you please provide sample language as to why symlinking the patch is even necessary? I imagine it starts with "The CMake build uses its own MODULE.bazel file ..."
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware
jwnimmer-tri
left a comment
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.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware
CMakeLists.txt line 300 at r5 (raw file):
Suggested comment along the lines of...
Put this patch where
cmake/MODULE.bazel.inexpects it during its call tosingle_version_override().
Amend 97465f7 with an approach that also works for externals using `@rules_cc//cc/compiler`, instead of a custom Drake flag.
33d7b9b to
3f97725
Compare
tyler-yankee
left a comment
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.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),mwoehlke-kitware
CMakeLists.txt line 300 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Suggested comment along the lines of...
Put this patch where
cmake/MODULE.bazel.inexpects it during its call tosingle_version_override().
thanks!
jwnimmer-tri
left a comment
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.
@jwnimmer-tri reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee mwoehlke-kitware
a discussion (no related file):
Needs platform reviewer assigned. Possibly it's OK to assign that now, concurrently with the full matrix testing?
|
I'm going to test the batch of CMake jobs from a prior revision again to make sure, without the drake-ci changes. @drake-jenkins-bot linux-jammy-gcc-cmake-experimental-release please |
tyler-yankee
left a comment
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.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees mwoehlke-kitware,SeanCurtis-TRI(platform)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Needs platform reviewer assigned. Possibly it's OK to assign that now, concurrently with the full matrix testing?
+a:@SeanCurtis-TRI for platform review (per schedule), please.
Amend #23117 with an approach that also works for externals using
@rules_cc//cc/compiler, instead of a custom Drake flag.Towards #23728.
This change is