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 configure_copts.bzl for non trasitive load in bazel 0.25 #301

Closed
wants to merge 1 commit into from

Conversation

jinmel
Copy link

@jinmel jinmel commented May 7, 2019

Fix copts for compatibility with bazel 0.25 update

bazelbuild/bazel#5636

@EricWF
Copy link
Contributor

EricWF commented May 7, 2019

Hi @jinmel,

Thanks for the fix. This looks good on our end. We're in the process of upgrading our bazel version internally, and I would like to hold off on merging this for day or two until we do that.

I'll check back with this Thursday.

@jinmel
Copy link
Author

jinmel commented May 8, 2019

@EricWF Let me know if there's any errors during upgrade. Thanks.

@EricWF
Copy link
Contributor

EricWF commented May 8, 2019

@jinmel We didn't see the errors this PR mentions after performing the upgrade.

It looks like the bazel changes that disable transitive loads were rolled back because it broke builds.
I'm assuming the plan is to re-commit it once the downstream projects fix their BUILD files to tolerate it?

Does that mean it won't make it into 0.25?

/Eric

@derekmauro
Copy link
Member

@jinmel I don't think this change is correct. I upgraded to bazel 0.25.0 and don't see any errors.

bazelbuild/bazel#5636 says

When a bzl file contains a load, e.g.
load("//some:file.bzl", "fct")
it implicitly exports fct.

But that's not what's happening here. The symbols this change is trying to re-export like ABSL_GCC_FLAGS aren't consumed directly by Abseil and aren't meant to be exported. For instance, ABSL_GCC_FLAGS is actually consumed through ABSL_DEFAULT_COPTS.

@jinmel
Copy link
Author

jinmel commented May 9, 2019

@derekmauro @EricWF Didn't know they weren't meant to be exported. I think there are quite some repositories using those flags.

The error was originally caused here: https://github.com/census-instrumentation/opencensus-cpp/blob/7c3607bb028beb1ce11b326c588e75c6cf19b094/opencensus/copts.bzl#L37

So over there we are trying to drop usage of internal flags and use DEFAULT_COPTS instead. But we are facing errors like this: census-instrumentation/opencensus-cpp@575ab11#r281957070

@jinmel
Copy link
Author

jinmel commented May 9, 2019

the select({...}) uses //absl:windows, //absl:llvm_compiler and importing this from other project causes error like: package 'absl' not found.

@derekmauro
Copy link
Member

What I meant was that ABSL_GCC_FLAGS isn't used directly by Abseil at all, and I'm not sure that we even meant to export ABSL_DEFAULT_COPTS to other projects, though I guess if you build Abseil, you implicitly get it because of how our build rules are currently setup.

I suspect you shouldn't be using ABSL_DEFAULT_COPTS at all in your project, and Abseil should find another way of setting our warning options so that we test with them, but don't export them to other projects that may not want certain warnings. I think the Bazel way setting build options is though a CROSSTOOL file, but those are notoriously difficult to setup and we don't use them ourselves.

@jinmel
Copy link
Author

jinmel commented May 10, 2019

@derekmauro so what you are saying is that we shouldn't import copts.bzl from other projects at all?

@derekmauro
Copy link
Member

I wouldn't import copts.bzl today, but I also think it is reasonable to request official support for the list of compiler warnings we use. We'd have to think about how to do this. If you want a quick fix, maybe you can import the options from https://github.com/abseil/abseil-cpp/blob/master/absl/copts/GENERATED_copts.bzl with the understanding that we might change the file in a way that breaks your project?

@jinmel
Copy link
Author

jinmel commented May 13, 2019

@asoffer
Copy link

asoffer commented May 13, 2019

@jinmel it looks like census-instrumentation/opencensus-cpp#316 resolves this issue for you, so I'm closing this PR. Please let us know if you have any concerns.

@asoffer asoffer closed this May 13, 2019
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.

5 participants