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

Remove linkstatic boilerplate. #5153

Closed

Conversation

david-german-tri
Copy link
Contributor

@david-german-tri david-german-tri commented Feb 13, 2017

Fixes #5104.

@jwnimmer-tri for both levels of review.


This change is Reviewable

@david-german-tri
Copy link
Contributor Author

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

@jwnimmer-tri
Copy link
Collaborator

Review status: 0 of 39 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


a discussion (no related file):
This PR removes linkstatic = 1 from call sites, but adds linkstatic = 1 only for cc_binary. I think most binary call sites did not already use linkstatic = 1, but most library call sites did use linkstatic = 1 but this PR undoes that.

I am curious to hear if those changes an intentional or an oversight?

Depending on that answer, I would also likely slightly prefer a first PR that just de-duplicates the logic but keeps the effective build rules (mostly) unchanged, and then a second PR that flips the defaults.


tools/drake.bzl, line 56 at r1 (raw file):

        linkstatic=1,
        **kwargs):
    """Creates a rule to declare a C++ binary."""

Probably the docs should explain the linker default?


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

Review status: 0 of 39 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This PR removes linkstatic = 1 from call sites, but adds linkstatic = 1 only for cc_binary. I think most binary call sites did not already use linkstatic = 1, but most library call sites did use linkstatic = 1 but this PR undoes that.

I am curious to hear if those changes an intentional or an oversight?

Depending on that answer, I would also likely slightly prefer a first PR that just de-duplicates the logic but keeps the effective build rules (mostly) unchanged, and then a second PR that flips the defaults.

No, that's a mistake, and explains the CI failure. Will close this and start over.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

a discussion (no related file):

Previously, david-german-tri (David German) wrote…

No, that's a mistake, and explains the CI failure. Will close this and start over.

This one is fairly close to being feature-neutral. Just needs somedrake.bzl edits, I think?


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants