Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Fix build options for non transitive load for bazel 0.25 #316

Merged
merged 9 commits into from
May 10, 2019

Conversation

jinmel
Copy link
Contributor

@jinmel jinmel commented May 7, 2019

@g-easy
Copy link
Contributor

g-easy commented May 7, 2019

Both with and without this change I'm seeing:

opencensus-cpp/opencensus/copts.bzl:24:1: file '@com_google_absl//absl:copts/configure_copts.bzl' does not contain symbol 'ABSL_GCC_FLAGS'

@jinmel
Copy link
Contributor Author

jinmel commented May 7, 2019

@g-easy absl-cpp also needs an update.

abseil/abseil-cpp#301

@g-easy
Copy link
Contributor

g-easy commented May 7, 2019

Ahh ok, I was trying it against abseil at HEAD.

Alternatively: should we just consume ABSL_DEFAULT_COPTS which is defined in absl/copts/configure_copts.bzl?

@jinmel jinmel force-pushed the non_transitive_load_fix branch from 89232a1 to 7c9fb01 Compare May 7, 2019 07:16
@jinmel
Copy link
Contributor Author

jinmel commented May 7, 2019

@g-easy consuming DEFAULT_COPTS directly seems better :)

btw, should I keep my commit to single commit per merge request? or is it squashed into one commit when I merge it. Just curious how it works in github

@g-easy
Copy link
Contributor

g-easy commented May 7, 2019

It all gets squashed so feel free to push as many commits as you'd like. :D

Also I try to clean up the final commit message / set it to the topic + initial comment.

opencensus/copts.bzl Outdated Show resolved Hide resolved
)

WERROR = ["-Werror=return-type", "-Werror=switch"]

DEFAULT_COPTS = select({
"//opencensus:llvm_compiler": ABSL_LLVM_FLAGS + WERROR,
"//opencensus:windows": ABSL_MSVC_FLAGS,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

DEFAULT_COPTS = select({
    "//opencensus:llvm_compiler": ABSL_DEFAULT_COPTS + WERROR,
    "//opencensus:windows": ABSL_DEFAULT_COPTS, # Without the gcc/clang specific WERROR
    "etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like select within select isn't possible

ERROR: /Users/jinsuk/Code/opencensus-cpp/opencensus/context/BUILD:24:1: //opencensus/context:context: expected value of type 'list(string)' for each branch in select expression of attribute 'copts' in 'cc_library' rule (including '//opencensus:llvm_compiler'), but got select({"//absl:windows": ["/W3", "/DNOMINMAX", "/DWIN32_LEAN_AND_MEAN", "/D_CRT_SECURE_NO_WARNINGS", "/D_SCL_SECURE_NO_WARNINGS", "/D_ENABLE_EXTENDED_ALIGNED_STORAGE", "/wd4005", "/wd4068", "/wd4180", "/wd4244", "/wd4267", "/wd4503", "/wd4800"], "//absl:llvm_compiler": ["-Wall", "-Wextra", "-Weverything", "-Wno-c++98-compat-pedantic", "-Wno-conversion", "-Wno-covered-switch-default", "-Wno-deprecated", "-Wno-disabled-macro-expansion", "-Wno-double-promotion", "-Wno-comma", "-Wno-extra-semi", "-Wno-extra-semi-stmt", "-Wno-packed", "-Wno-padded", "-Wno-sign-compare", "-Wno-float-conversion", "-Wno-float-equal", "-Wno-format-nonliteral", "-Wno-gcc-compat", "-Wno-global-constructors", "-Wno-exit-time-destructors", "-Wno-nested-anon-types", "-Wno-non-modular-include-in-module", "-Wno-old-style-cast", "-Wno-range-loop-analysis", "-Wno-reserved-id-macro", "-Wno-shorten-64-to-32", "-Wno-switch-enum", "-Wno-thread-safety-negative", "-Wno-unknown-warning-option", "-Wno-unreachable-code", "-Wno-unused-macros", "-Wno-weak-vtables", "-Wbitfield-enum-conversion", "-Wbool-conversion", "-Wconstant-conversion", "-Wenum-conversion", "-Wint-conversion", "-Wliteral-conversion", "-Wnon-literal-null-conversion", "-Wnull-conversion", "-Wobjc-literal-conversion", "-Wno-sign-conversion", "-Wstring-conversion"], "//conditions:default": ["-Wall", "-Wextra", "-Wcast-qual", "-Wconversion-null", "-Wmissing-declarations", "-Woverlength-strings", "-Wpointer-arith", "-Wunused-local-typedefs", "-Wunused-result", "-Wvarargs", "-Wvla", "-Wwrite-strings", "-Wno-missing-field-initializers", "-Wno-sign-compare"]}) + ["-Werror=return-type", "-Werror=switch"] (select)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@g-easy
https://github.com/abseil/abseil-cpp/blob/aa468ad75539619b47979911297efbb629c52e44/absl/copts/configure_copts.bzl#L30

It seems like using ABSL_DEFAULT_COPTS causes error because it queries //absl:windows of which 'absl' package is not found in opencensus-cpp directory :(

Copy link
Contributor

Choose a reason for hiding this comment

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

select within select isn't possible
I didn't know this. It's unfortunate but unsurprising.

How about:

DEFAULT_COPTS = ABSL_DEFAULT_COPS + WARN_OPTS

Where:

WARN_OPTS = select(...

Would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this what you meant?
I think the select causing error is within ABSL_DEFAULT_COPTS.. I wonder if this variable is usable anywhere

Copy link
Contributor Author

@jinmel jinmel left a comment

Choose a reason for hiding this comment

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

There still needs to be an update in abseil-cpp side.

Copy link
Contributor Author

@jinmel jinmel left a comment

Choose a reason for hiding this comment

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

Just addressed your comment

@g-easy
Copy link
Contributor

g-easy commented May 9, 2019

IMO let's do a minimal build fix for now, which is this:

 load(
-    "@com_google_absl//absl:copts/configure_copts.bzl",
+    "@com_google_absl//absl:copts/GENERATED_copts.bzl",

Longer term we should probably depend on ABSL_DEFAULT_COPTS from configure_copts.bzl but it looks like there's an error in how bazel handles select() across external repositories.

Another possible later solution is for opencensus-cpp to maintain its own set of flags. We initially went with Abseil's because we use the same style guide (which results in a lots of sign conversion warnings).

Could you please make your PR just change that load()?

@jinmel
Copy link
Contributor Author

jinmel commented May 9, 2019

abseil/abseil-cpp#301 (comment)

It seems like those ABSL_LLVM* kind of flags weren't meant to be exported.

They instruct us to use ABSL_DEFAULT_COPTS which isn't usable :D

Copy link
Contributor

@g-easy g-easy left a comment

Choose a reason for hiding this comment

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

I'm working on fixing the format.sh problem. :)

opencensus/copts.bzl Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants