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

Fix compiler identification for Bazel 1.1.0 on macOS #12224

Merged
merged 2 commits into from Oct 22, 2019
Merged

Fix compiler identification for Bazel 1.1.0 on macOS #12224

merged 2 commits into from Oct 22, 2019

Conversation

jamiesnape
Copy link
Contributor

@jamiesnape jamiesnape commented Oct 21, 2019

Once everyone is on Bazel 1.1.0 or above, much of this file can be simplified.


This change is Reviewable

@jamiesnape
Copy link
Contributor Author

@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please

@jamiesnape
Copy link
Contributor Author

Bazel 1.0 is past the compiler check, so everything is good.

@jamiesnape
Copy link
Contributor Author

+@soonho-tri for feature review and +@jwnimmer-tri for platform review.

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: platform.

Reviewed 1 of 1 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee soonho-tri(platform) (waiting on @jamiesnape and @soonho-tri)


tools/workspace/cc/repository.bzl, line 59 at r1 (raw file):

        cc = repository_ctx.path(Label("@local_config_cc//:wrapped_clang"))

        result = repository_ctx.execute(["xcode-select", "--print-path"])

nit For brevity, PATH soundness, and more careful error messages -- consider using execute_or_fail from //tools/workspace:execute.bzl here. OK to dismiss.

Ditto below.

@jamiesnape
Copy link
Contributor Author

We had failing tests too. Fixing those properly is a little more involved, so I pushed a temporary patch.

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 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee soonho-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jamiesnape and @soonho-tri)

Copy link
Member

@soonho-tri soonho-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:

Reviewed 1 of 1 files at r1, 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jamiesnape)

@jamiesnape jamiesnape added the status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits label Oct 22, 2019
@jamiesnape
Copy link
Contributor Author

Note Starlark functions apple_common.apple_toolchain().developer_dir() and apple_common.apple_toolchain().sdk_dir() are returning useless placeholders for whatever reason.

Copy link
Contributor Author

@jamiesnape jamiesnape 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: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),soonho-tri(platform) (waiting on @jwnimmer-tri and @soonho-tri)


tools/workspace/cc/repository.bzl, line 59 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit For brevity, PATH soundness, and more careful error messages -- consider using execute_or_fail from //tools/workspace:execute.bzl here. OK to dismiss.

Ditto below.

Done.

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 2 of 2 files at r3.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),soonho-tri(platform)

@jamiesnape jamiesnape merged commit 26a998d into RobotLocomotion:master Oct 22, 2019
@jamiesnape jamiesnape deleted the fix-mac-bazel-1.1 branch October 22, 2019 15:13
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.

None yet

3 participants