Fix build options for non transitive load for bazel 0.25#316
Fix build options for non transitive load for bazel 0.25#316g-easy merged 9 commits intocensus-instrumentation:masterfrom
Conversation
|
Both with and without this change I'm seeing: |
|
@g-easy absl-cpp also needs an update. |
|
Ahh ok, I was trying it against abseil at HEAD. Alternatively: should we just consume |
89232a1 to
7c9fb01
Compare
|
@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 |
|
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. |
|
|
||
| DEFAULT_COPTS = select({ | ||
| "//opencensus:llvm_compiler": ABSL_LLVM_FLAGS + WERROR, | ||
| "//opencensus:windows": ABSL_MSVC_FLAGS, |
There was a problem hiding this comment.
How about:
DEFAULT_COPTS = select({
"//opencensus:llvm_compiler": ABSL_DEFAULT_COPTS + WERROR,
"//opencensus:windows": ABSL_DEFAULT_COPTS, # Without the gcc/clang specific WERROR
"etc
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@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 :(
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
is this what you meant?
I think the select causing error is within ABSL_DEFAULT_COPTS.. I wonder if this variable is usable anywhere
jinmel
left a comment
There was a problem hiding this comment.
There still needs to be an update in abseil-cpp side.
jinmel
left a comment
There was a problem hiding this comment.
Just addressed your comment
|
IMO let's do a minimal build fix for now, which is this: Longer term we should probably depend on 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 |
|
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 |
g-easy
left a comment
There was a problem hiding this comment.
I'm working on fixing the format.sh problem. :)
bazelbuild/bazel#5636