Skip to content

Conversation

@tyler-yankee
Copy link
Contributor

@tyler-yankee tyler-yankee commented Nov 11, 2025

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 Reviewable

@tyler-yankee

This comment was marked as outdated.

@tyler-yankee
Copy link
Contributor Author

@drake-jenkins-bot linux-jammy-gcc-cmake-experimental-release please
@drake-jenkins-bot linux-noble-unprovisioned-gcc-cmake-experimental-packaging please
@drake-jenkins-bot mac-arm-sequoia-clang-cmake-experimental-release please
@drake-jenkins-bot linux-noble-unprovisioned-gcc-wheel-experimental-release please

@jwnimmer-tri jwnimmer-tri self-assigned this Nov 12, 2025
@jwnimmer-tri jwnimmer-tri added the release notes: fix This pull request contains fixes (no new features) label Nov 12, 2025
Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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 is

CMakeLists.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?

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 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?

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.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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).

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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-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).

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?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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?

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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 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?

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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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_config on 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!

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 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?

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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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 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.

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.

@jwnimmer-tri jwnimmer-tri changed the title [build] Skirt around compiler introspection for CMake builds [build] Rely on CMake for compiler introspection Nov 14, 2025
Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 foo doesn't change anything (so it's not somehow confusing the GCC executable with the local version)
  • making an additional symlink ld -> /usr/bin/ld doesn't change anything (so GCC isn't looking for an ld alongside it / in the same directory)
  • /usr/bin is on PATH

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 foo doesn't change anything (so it's not somehow confusing the GCC executable with the local version)
  • making an additional symlink ld -> /usr/bin/ld doesn't change anything (so GCC isn't looking for an ld alongside it / in the same directory)
  • /usr/bin is on PATH

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.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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.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.

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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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?

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 readlink until 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?)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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 mold but 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:

https://github.com/bazelbuild/rules_cc/blob/d0fa9b7ad62718bc1cb8ebb2f546d6651cf88ab7/cc/private/toolchain/unix_cc_configure.bzl#L272-L286

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?

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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:

https://github.com/bazelbuild/rules_cc/blob/d0fa9b7ad62718bc1cb8ebb2f546d6651cf88ab7/cc/private/toolchain/unix_cc_configure.bzl#L272-L286

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.

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)

@tyler-yankee tyler-yankee changed the title [build] Rely on CMake for compiler introspection [build] Patch rules_cc to recognize cc as gcc Nov 18, 2025
Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 -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.

Thanks. I only checked cc -> clang for Apple Clang, should have also checked on Linux.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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.

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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 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.

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 -> clang for Apple Clang, should have also checked on Linux.

I've adapted the condition to be more strict.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware left a 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:compiler to 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

@jwnimmer-tri
Copy link
Collaborator

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.

Create symlink ${DRAKE_BAZEL_BUILD_DIR}/cc-as-gcc.patch. It isn't clear why this is needed, ...

@tyler-yankee let's add a comment in that case, to clarify.

Copy link
Contributor

@mwoehlke-kitware mwoehlke-kitware left a 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

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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:compiler to 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

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a 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.in expects it during its call to single_version_override().

Amend 97465f7 with an approach that also works for externals using
`@rules_cc//cc/compiler`, instead of a custom Drake flag.
Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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.in expects it during its call to single_version_override().

thanks!

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: feature

@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?

@tyler-yankee
Copy link
Contributor Author

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
@drake-jenkins-bot linux-noble-unprovisioned-gcc-cmake-experimental-packaging please
@drake-jenkins-bot mac-arm-sequoia-clang-cmake-experimental-release please
@drake-jenkins-bot linux-noble-unprovisioned-gcc-wheel-experimental-release please

Copy link
Contributor Author

@tyler-yankee tyler-yankee left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: fix This pull request contains fixes (no new features)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants