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

Propagate restricted_to and compatible_with attributes in cgo rules #1493

Merged
merged 1 commit into from May 9, 2018
Merged

Conversation

iley
Copy link
Contributor

@iley iley commented May 9, 2018

This fixes #1488.

@iley
Copy link
Contributor Author

iley commented May 9, 2018

I'll have a look at the failing build.

Copy link
Contributor

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this! Looks good, with a couple small suggestions.

@@ -523,6 +523,8 @@ def setup_cgo_library(name, srcs, cdeps, copts, cxxopts, cppopts, clinkopts, obj

if objc:
cgo_objc_lib_name = name + ".cgo_objc_lib"
kwargs = extra_args
kwargs.update(objcopts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to update objcopts with extra_args here instead of updating extra_args. This statement modifies extra_args, which is used in other rules below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right! Thanks for catching this.

@@ -397,7 +397,7 @@ _cgo_collect_info = go_rule(
"""No-op rule that collects information from _cgo_codegen and cc_library
info into a GoSourceList provider for easy consumption."""

def setup_cgo_library(name, srcs, cdeps, copts, cxxopts, cppopts, clinkopts, objc, objcopts, tags):
def setup_cgo_library(name, srcs, cdeps, copts, cxxopts, cppopts, clinkopts, objc, objcopts, **extra_args):
Copy link
Contributor

Choose a reason for hiding this comment

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

One small nit: rename extra_args to common_attrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Done.

@jayconrod jayconrod merged commit 3aedd54 into bazelbuild:master May 9, 2018
@jayconrod
Copy link
Contributor

Thanks!

The Travis failure appears to be a problem with Travis CI itself, not this change. Tests pass locally and on Buildkite, so force submitting.

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

Successfully merging this pull request may close these issues.

Platform constraints don't work with cgo
3 participants