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

Update osx toolchain to match C/C++ part of latest upstream toolchain #7761

Merged
merged 1 commit into from
Jan 16, 2018
Merged

Update osx toolchain to match C/C++ part of latest upstream toolchain #7761

merged 1 commit into from
Jan 16, 2018

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented Jan 15, 2018

Relates in part to RobotLocomotion/drake-external-examples#78. Also significantly improves #5183.

The amount of code in the new CROSSTOOL really does illustrate the difficulty of using a custom toolchain while they update the format of the CROSSTOOL file in upstream bazel. Basically, the current situation is that some of the build settings use the legacy format and some use the new feature/action-based format. Bazel does attempt to generate various parts of the new format from the old, but it is not perfect on macOS. Hence, our macOS toolchain was both out-of-date and internally inconsistent.


This change is Reviewable

@jamiesnape
Copy link
Contributor Author

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-memcheck-asan please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-memcheck-tsan please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-snopt please
@drake-jenkins-bot mac-highsierra-clang-cmake-experimental please
@drake-jenkins-bot mac-highsierra-clang-cmake-experimental-snopt please
@drake-jenkins-bot mac-highsierra-unprovisioned-clang-bazel-experimental please
@drake-jenkins-bot mac-highsierra-unprovisioned-clang-bazel-experimental-snopt-packaging please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental-memcheck-asan please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental-memcheck-tsan please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental-snopt please
@drake-jenkins-bot mac-sierra-clang-cmake-experimental please
@drake-jenkins-bot mac-sierra-clang-cmake-experimental-matlab please
@drake-jenkins-bot mac-sierra-clang-cmake-experimental-matlab-snopt please
@drake-jenkins-bot mac-sierra-clang-cmake-experimental-snopt please
@drake-jenkins-bot mac-sierra-unprovisioned-clang-bazel-experimental please

@jamiesnape
Copy link
Contributor Author

jamiesnape commented Jan 16, 2018

@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-highsierra-unprovisioned-clang-bazel-experimental please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental please
@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-sierra-unprovisioned-clang-bazel-experimental please

@jamiesnape
Copy link
Contributor Author

+@soonho-tri for feature review. Please test locally. Every combination of CI job that we have for Mac has passed.

@soonho-tri
Copy link
Member

Reviewed 2 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.


tools/cc_toolchain/osx_ar_wrapper.sh, line 5 at r1 (raw file):

set -euo pipefail

export ZERO_AR_DATE=1

Could you add a note to this line (i.e. why we need this?)


Comments from Reviewable

@soonho-tri
Copy link
Member

:lgtm: Local tests passed. I couldn't checked the changes in CROSSTOOL thoroughly though.


Reviewed 1 of 3 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@jamiesnape
Copy link
Contributor Author

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.


tools/cc_toolchain/osx_ar_wrapper.sh, line 5 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Could you add a note to this line (i.e. why we need this?)

Done.


Comments from Reviewable

@jamiesnape
Copy link
Contributor Author

+@sammy-tri for platform review.

@soonho-tri
Copy link
Member

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sammy-tri
Copy link
Contributor

I don't really know how to review the CROSSTOOL changes. I think I could probably update it using the provided instructions if I absolutely had to, so maybe that's enough? :lgtm: since it doesn't seem to be any less reproducible (and is if anything better documented) than the old builds and CI seems happy.


Reviewed 1 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jamiesnape jamiesnape merged commit 56e8bfd into RobotLocomotion:master Jan 16, 2018
@jamiesnape jamiesnape deleted the osx-toolchain branch January 16, 2018 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants