Skip to content
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

[doc] x86_64 macOS monterey uses Xcode 14 #18005

Merged

Conversation

svenevs
Copy link
Contributor

@svenevs svenevs commented Sep 29, 2022

Binary artifacts produced by drake (nightly .tar.gz and releases) are now being compiled with Xcode 14.0.1.

Relates: #17985.

Note: release notes label may be incorrect here.


This change is Reviewable

Copy link
Contributor Author

@svenevs svenevs 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: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"


doc/_pages/mac.md line 7 at r1 (raw file):

We assume that you have already installed:

* Xcode 10 or above ([from the Mac App Store](https://itunes.apple.com/us/app/xcode/id497799835))

FYI I deleted this since the only way people get to this page is by navigating to it from the from_source page which has the supported versions of xcode. Stating 12 or above would be misleading since it depends on big sur vs monterey.

Copy link
Contributor Author

@svenevs svenevs 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, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @svenevs)

a discussion (no related file):
Working, currently we don't split "minimum compiler version checks" (bazel or cmake) into os-level constraints. Right now we would accept, for example, xcode 12 on monterey

supported_compilers = {"AppleClang": (12, 0)}

If I was understanding the slack discussion then we'd want to use a different minimum version number (encode the table in from_source)?


@jwnimmer-tri jwnimmer-tri self-assigned this Sep 29, 2022
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.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @svenevs)

a discussion (no related file):

Previously, svenevs (Stephen McDowell) wrote…

Working, currently we don't split "minimum compiler version checks" (bazel or cmake) into os-level constraints. Right now we would accept, for example, xcode 12 on monterey

supported_compilers = {"AppleClang": (12, 0)}

If I was understanding the slack discussion then we'd want to use a different minimum version number (encode the table in from_source)?

I think it would be fair to keep just one macOS Clang version, rather adding the code complexity to split it by platform. It's just a disclaimer anyway; it doesn't need to be precise. If we keep it at the lowest-supported (currently AppleClang 12) then it won't have false positives. If someone runs Xcode 12 on Monterey it will probably work anyway, i.e., I'm not too worried about false negatives. We're just giving the "AppleClang < 12" warning so that people with a really old Xcode will have a hint about why it might not be working. Our official support policy is whatever the website docs say, not the CMakeLists or repository rule warning logs.



doc/_pages/from_source.md line 22 at r1 (raw file):

| macOS Big Sur (11)                 | x86_64       | 3.10 ⁽²⁾ | 5.1   | 3.24  | Apple LLVM 12.0.5 (Xcode 12.5)   | AdoptOpenJDK 16 (HotSpot JVM) |
| macOS Monterey (12)                | x86_64       | 3.10 ⁽²⁾ | 5.1   | 3.24  | Apple LLVM 14.0.0 (Xcode 14.0.1) | AdoptOpenJDK 16 (HotSpot JVM) |
| macOS Monterey (12)                | arm64 ⁽¹⁾    | 3.10 ⁽²⁾ | 5.1   | 3.24  | Apple LLVM 13.1.6 (Xcode 13.4)   | AdoptOpenJDK 16 (HotSpot JVM) |

I wonder whether patch-level version numbers here are buying us any value -- they seem like a pain to keep up to date.

Even the GCC 9.3 is out of date now (Ubuntu has since switched to 9.4).

Maybe we should only list the major numbers here across the board? (GCC 9, GCC 11, Clang 12, LLVM 12 (Xcode 12), etc.). It would be making the detailed version information more difficult to find (need to skim through CI logs), but I'm not sure that anyone cares.

WDYT?

@svenevs
Copy link
Contributor Author

svenevs commented Sep 30, 2022

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-documentation please.

Copy link
Contributor Author

@svenevs svenevs 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, labeled "do not merge" (waiting on @jwnimmer-tri and @svenevs)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think it would be fair to keep just one macOS Clang version, rather adding the code complexity to split it by platform. It's just a disclaimer anyway; it doesn't need to be precise. If we keep it at the lowest-supported (currently AppleClang 12) then it won't have false positives. If someone runs Xcode 12 on Monterey it will probably work anyway, i.e., I'm not too worried about false negatives. We're just giving the "AppleClang < 12" warning so that people with a really old Xcode will have a hint about why it might not be working. Our official support policy is whatever the website docs say, not the CMakeLists or repository rule warning logs.

OK, works for me 👍



doc/_pages/from_source.md line 22 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I wonder whether patch-level version numbers here are buying us any value -- they seem like a pain to keep up to date.

Even the GCC 9.3 is out of date now (Ubuntu has since switched to 9.4).

Maybe we should only list the major numbers here across the board? (GCC 9, GCC 11, Clang 12, LLVM 12 (Xcode 12), etc.). It would be making the detailed version information more difficult to find (need to skim through CI logs), but I'm not sure that anyone cares.

WDYT?

Done, I agree -- we really should only have to worry about the compiler major version.

@svenevs
Copy link
Contributor Author

svenevs commented Sep 30, 2022

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-documentation please.

Needed to rebase and resolve a merge conflict.

@svenevs
Copy link
Contributor Author

svenevs commented Sep 30, 2022

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-documentation please.

Gah! Sorry for spam, the bazel 5.3 update was the issue but I reverted that in the previous rebase -- diff here looks good now...

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:

I am not completely sure about "breaking change", but in any case we can leave it at that more conservative posture for now and then rework it in the notes document itself.

+@rpoyner-tri for platform review, please.

Reviewed 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform) (waiting on @rpoyner-tri and @svenevs)


doc/_pages/from_source.md line 19 at r4 (raw file):

|------------------------------------|--------------|----------|-------|-------|--------------------------|-------------------------------|
| Ubuntu 20.04 LTS (Focal Fossa)     | x86_64       | 3.8 ⁽²⁾  | 5.3   | 3.16  | GCC 9 or Clang 12        | OpenJDK 11                    |
| Ubuntu 22.04 LTS (Jammy Jellyfish) | x86_64       | 3.10 ⁽²⁾ | 5.3   | 3.22  | GCC 11 or Clang 12       | OpenJDK 11                    |

nit I think the "(default)" here is important to keep around (for both Ubuntu rows).

Suggestion:

GCC 11 (default)

Copy link
Contributor

@rpoyner-tri rpoyner-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: pending the one discussion.

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @svenevs)

Binary artifacts produced by drake (nightly `.tar.gz` and releases)
are now being compiled with Xcode 14.0.1.
Copy link
Contributor Author

@svenevs svenevs left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)


doc/_pages/from_source.md line 19 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I think the "(default)" here is important to keep around (for both Ubuntu rows).

OK, (default) restored!

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.

Reviewed all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @svenevs)

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.

Reviewed 1 of 1 files at r5.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform) (waiting on @svenevs)

@jwnimmer-tri jwnimmer-tri merged commit 5b86935 into RobotLocomotion:master Sep 30, 2022
@svenevs svenevs deleted the doc/xcode-14-x86_64-support branch September 30, 2022 17:12
@svenevs svenevs mentioned this pull request Sep 30, 2022
3 tasks
@jwnimmer-tri
Copy link
Collaborator

Since our packages come from Big Sur anyway, I'm going to demote this away from "breaking change".

@jwnimmer-tri jwnimmer-tri added release notes: fix This pull request contains fixes (no new features) and removed release notes: breaking change This pull request contains breaking changes labels Oct 7, 2022
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.

None yet

3 participants