[setup] Add Xcode 16 specific build configuration #22227

rpoyner-tri · 2024-11-21T19:57:29Z

Closes #22204.

This change is

rpoyner-tri · 2024-11-21T20:00:03Z

@drake-jenkins-bot mac-arm-sequoia-unprovisioned-clang-bazel-experimental-release please.

rpoyner-tri · 2024-11-21T22:34:23Z

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

@drake-jenkins-bot mac-arm-sequoia-unprovisioned-clang-bazel-experimental-release please.

@drake-jenkins-bot mac-arm-sequoia-unprovisioned-clang-bazel-experimental-release please.

rpoyner-tri

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri)

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

@drake-jenkins-bot mac-arm-sequoia-unprovisioned-clang-bazel-experimental-release please.

-(status: do not merge) -(status: do not review)
+(release notes: none) +@jwnimmer-tri for feature review, or both reviews if you like.

a discussion (no related file):
This patch doesn't fix [//examples/hardware\_sim:py/hardware\_sim\_py\_test](https://drake-cdash.csail.mit.edu/test/1638523679) on the sequoia CI image; I suspect that image is missing a routing table entry for LCM.

rpoyner-tri

Closes: #22204.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

jwnimmer-tri

Closes: `#22204.

BTW Posting this as a comment does not engage GitHub's auto-close logic. I edited the PR overview with this text, so that it have immediate effect.

Reviewed 1 of 3 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

setup/mac/source_distribution/install_prereqs_user_environment.sh line 19 at r2 (raw file):

bazelrc="${workspace_dir}/gen/environment.bazelrc"
arch="$(/usr/bin/arch)"
clang_version="$(clang --version |head -n1 |sed 's#.*(clang-\([0-9]*\)\..*#\1#g')"

I am skeptical that conditionalizing our setup script is worth the complexity cost (possibility of failure). Instead, it seems to me that we could declare XCode >= 16 as our only supported version moving forward (i.e., in the nightly build, and then released as of Drake 1.36). In that case, we would add the new -fno-assume... line to the macos.bazelrc directly.

setup/mac/source_distribution/install_prereqs_user_environment.sh line 29 at r2 (raw file):

if (( $clang_version >= 1600 )); then
cat >> "${bazelrc}" <<EOF
import %workspace%/tools/apple-clang-1600.bazelrc

The install_prereqs_user_environment.sh is not run during CMake builds. Instead, its logic is re-implemented fresh:

drake/CMakeLists.txt

Lines 44 to 55 in 1c5e341

    
             list(APPEND BAZELRC_IMPORTS "tools/macos.bazelrc") 
        
             execute_process( 
        
               COMMAND "/usr/bin/arch" 
        
               OUTPUT_STRIP_TRAILING_WHITESPACE 
        
               OUTPUT_VARIABLE MACOS_ARCH) 
        
             if(MACOS_ARCH STREQUAL "") 
        
               message(FATAL_ERROR "Could NOT query macOS arch") 
        
             endif() 
        
             list(APPEND BAZELRC_IMPORTS "tools/macos-arch-${MACOS_ARCH}.bazelrc") 
        
           else() 
        
             list(APPEND BAZELRC_IMPORTS "tools/ubuntu.bazelrc")

If we keep the version-probing logic, then we need to do it in both places.

rpoyner-tri

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

setup/mac/source_distribution/install_prereqs_user_environment.sh line 19 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am skeptical that conditionalizing our setup script is worth the complexity cost (possibility of failure). Instead, it seems to me that we could declare XCode >= 16 as our only supported version moving forward (i.e., in the nightly build, and then released as of Drake 1.36). In that case, we would add the new -fno-assume... line to the macos.bazelrc directly.

flag day it is, then.

rpoyner-tri

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

setup/mac/source_distribution/install_prereqs_user_environment.sh line 19 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

flag day it is, then.

Done.

setup/mac/source_distribution/install_prereqs_user_environment.sh line 29 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The install_prereqs_user_environment.sh is not run during CMake builds. Instead, its logic is re-implemented fresh:

drake/CMakeLists.txt

Lines 44 to 55 in 1c5e341

list(APPEND BAZELRC_IMPORTS "tools/macos.bazelrc")

execute_process(

COMMAND "/usr/bin/arch"

OUTPUT_STRIP_TRAILING_WHITESPACE

OUTPUT_VARIABLE MACOS_ARCH)

if(MACOS_ARCH STREQUAL "")

message(FATAL_ERROR "Could NOT query macOS arch")

endif()

list(APPEND BAZELRC_IMPORTS "tools/macos-arch-${MACOS_ARCH}.bazelrc")

else()

list(APPEND BAZELRC_IMPORTS "tools/ubuntu.bazelrc")

If we keep the version-probing logic, then we need to do it in both places.

Done.

rpoyner-tri

-(release notes: none) +(release notes: fix) Probably needs to be in "Announcements" section of release notes as well.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

rpoyner-tri

Sonoma CI will fail until images are re-spun with Xcode 16 or later.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

jwnimmer-tri

feature.

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

doc/_pages/installation.md line 69 at r3 (raw file):

| Ubuntu 24.04 LTS (Noble Numbat)    | GCC 13                   | C++20 |
| macOS Sonoma (14)                  | Apple LLVM 16 (Xcode 16) | C++20 |
| macOS Sequoia (15)                 | Apple LLVM 16 (Xcode 16) | C++20 |

nit I think it's fine for Sequoia row in the table say either TBD or XCode 16 for the compiler, but whichever one we choose we should be consistent between installation.md and from_source.md. Right now we have XCode 16 here and TBD in from_source.md.

tools/macos.bazelrc line 16 at r3 (raw file):


# https://github.com/RobotLocomotion/drake/issues/22204
build:clang --copt=-fno-assume-unique-vtables

nit The user passing --config=clang on the command line is not required. We need to compile correctly by default without any flags on the command line. (See my suggested edit, below.)

Yes, the "Options for explicitly using Clang" stanza below is nonsense. Ideally, our macOS CI scripting would not be passing --config=clang so that CI would have caught this bug for us. If you are eager, you're invited to PR to https://github.com/RobotLocomotion/drake-ci with that fix as well, though I'm also fine if you'd rather just ignore it.

Suggestion:

build --copt=-fno-assume-unique-vtables

Turn off a new optimization for dynamic_cast that is incompatible with loading multiple shared libraries into a python process. This patch effectively abandons support for Xcode versions older than 16. Update the compiler version advice for macOS configurations, and touch up some inconsistencies.

rpoyner-tri

+@xuchenhan-tri for platform review, Monday schedule.

Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)

jwnimmer-tri

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)

xuchenhan-tri

BTW, does this warrant a slack message in the DrakeDevelopers workspace in case this immediately affects developers on macOS?

Reviewed 2 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),xuchenhan-tri(platform)

jwnimmer-tri · 2024-11-25T20:10:28Z

Yes. However, the bigger picture is that we can't merge this until new CI images are ready, which is estimated to be a couple of weeks away.

xuchenhan-tri

Good to know, thanks!

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

rpoyner-tri added status: do not merge status: do not review labels Nov 21, 2024

rpoyner-tri force-pushed the xcode-16-build-flags branch 2 times, most recently from bf0f471 to 71b50c8 Compare November 21, 2024 22:26

rpoyner-tri mentioned this pull request Nov 21, 2024

xcode 16 + python + multiple shared libraries + dynamic_cast ==> fail #22204

Open

rpoyner-tri force-pushed the xcode-16-build-flags branch from 71b50c8 to 22d9f96 Compare November 22, 2024 03:12

rpoyner-tri changed the title ~~wip~~ [setup] Add Xcode 16 specific build configuration Nov 22, 2024

rpoyner-tri added release notes: none This pull request should not be mentioned in the release notes and removed status: do not merge status: do not review labels Nov 22, 2024

rpoyner-tri assigned jwnimmer-tri Nov 22, 2024

rpoyner-tri commented Nov 22, 2024

View reviewed changes

rpoyner-tri marked this pull request as ready for review November 22, 2024 03:17

rpoyner-tri mentioned this pull request Nov 22, 2024

[multibody] Remove dynamic_cast from inline get-downcast functions #22209

Open

rpoyner-tri commented Nov 22, 2024

View reviewed changes

jwnimmer-tri reviewed Nov 22, 2024

View reviewed changes

rpoyner-tri commented Nov 23, 2024

View reviewed changes

rpoyner-tri force-pushed the xcode-16-build-flags branch from 22d9f96 to 1303bb0 Compare November 23, 2024 16:15

rpoyner-tri commented Nov 23, 2024

View reviewed changes

rpoyner-tri force-pushed the xcode-16-build-flags branch from 1303bb0 to a5a08dc Compare November 23, 2024 16:20

rpoyner-tri added release notes: fix This pull request contains fixes (no new features) and removed release notes: none This pull request should not be mentioned in the release notes labels Nov 23, 2024

rpoyner-tri commented Nov 23, 2024

View reviewed changes

jwnimmer-tri reviewed Nov 23, 2024

View reviewed changes

rpoyner-tri force-pushed the xcode-16-build-flags branch from a5a08dc to b27e23f Compare November 23, 2024 22:09

rpoyner-tri assigned xuchenhan-tri Nov 23, 2024

rpoyner-tri commented Nov 23, 2024

View reviewed changes

jwnimmer-tri reviewed Nov 24, 2024

View reviewed changes

jwnimmer-tri mentioned this pull request Nov 25, 2024

Upgrade to XCode 16 #22031

Open

xuchenhan-tri reviewed Nov 25, 2024

View reviewed changes

jwnimmer-tri added the status: do not merge label Nov 25, 2024

xuchenhan-tri reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[setup] Add Xcode 16 specific build configuration #22227

[setup] Add Xcode 16 specific build configuration #22227

rpoyner-tri commented Nov 21, 2024 •

edited by jwnimmer-tri

Loading

rpoyner-tri commented Nov 21, 2024

rpoyner-tri commented Nov 21, 2024

rpoyner-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

rpoyner-tri left a comment

rpoyner-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

xuchenhan-tri left a comment

jwnimmer-tri commented Nov 25, 2024

xuchenhan-tri left a comment

	list(APPEND BAZELRC_IMPORTS "tools/macos.bazelrc")

	execute_process(
	COMMAND "/usr/bin/arch"
	OUTPUT_STRIP_TRAILING_WHITESPACE
	OUTPUT_VARIABLE MACOS_ARCH)
	if(MACOS_ARCH STREQUAL "")
	message(FATAL_ERROR "Could NOT query macOS arch")
	endif()
	list(APPEND BAZELRC_IMPORTS "tools/macos-arch-${MACOS_ARCH}.bazelrc")
	else()
	list(APPEND BAZELRC_IMPORTS "tools/ubuntu.bazelrc")

[setup] Add Xcode 16 specific build configuration #22227

Are you sure you want to change the base?

[setup] Add Xcode 16 specific build configuration #22227

Conversation

rpoyner-tri commented Nov 21, 2024 • edited by jwnimmer-tri Loading

rpoyner-tri commented Nov 21, 2024

rpoyner-tri commented Nov 21, 2024

rpoyner-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

xuchenhan-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Nov 25, 2024

xuchenhan-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri commented Nov 21, 2024 •

edited by jwnimmer-tri

Loading