-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
skylark: Prefer @drake//tools/skylark
over //tools/skylark
#9845
skylark: Prefer @drake//tools/skylark
over //tools/skylark
#9845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@jwnimmer-tri for review, please.
Reviewable status: all discussions resolved, platform LGTM missing
I think it would make more sense to kick this to on-call review. The point of me punting on it was to reduce my distractions. |
Do we (want to) have a Bazel style guide to start to document all these workarounds? I can't keep track of them all anymore. |
I think some conventions probably deserve a styleguide, but for this (functional) rule I think a linter would be more effective. |
Certainly more effective, but it would nice for humans to have something to go by as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-@jwnimmer-tri for review
+@sammy-tri for both reviews (low priority, so whenevs).
Will make a separate issue on how to best handle this in the future.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 40 of 40 files at r1.
Reviewable status: complete! all discussions resolved, platform LGTM from [sammy-tri]
For more information, see: - bazelbuild/bazel#3115 - RobotLocomotion#8700
a0f1cda
to
086e4cb
Compare
There was a problem hiding this 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 r2.
Reviewable status: complete! all discussions resolved, platform LGTM from [sammy-tri]
For more information, see:
@
and@workspacename
considered different repositories bazelbuild/bazel#3115common/proto/BUILD.bazel
install rules are sensitive to whether they use//tools:drake.bzl
vs.//tools/skylark:drake_cc.bzl
#8700Resolves #8732
This change is