-
Notifications
You must be signed in to change notification settings - Fork 76
Conversation
opencensus/copts.bzl
Outdated
@@ -33,7 +33,7 @@ load( | |||
|
|||
DEFAULT_COPTS = select({ | |||
"//opencensus:llvm_compiler": LLVM_FLAGS, | |||
"//opencensus:windows": MSVC_FLAGS, | |||
"//opencensus:windows": MSVC_FLAGS + ["/wd4715"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment about what 4715 is.
Also: not needed in TEST_COPTS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented ("not all control paths return a value", treated as an error). I added it to TEST_COPTS for good measure--I do not think any of our code exercises that warning.
|
||
REM TODO: Is there an easier way to convert lines to a space-separated list? | ||
SET BUILDABLES= | ||
FOR /F usebackq %%T IN (`bazel query "kind(rule, //...)" ^| FINDSTR /C:"\:_" /V`) DO ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
tools/appveyor/build.bat
Outdated
|
||
REM TODO: Remove --output_user_root after https://github.com/bazelbuild/bazel/issues/4149 is fixed. | ||
bazel --output_user_root=c:/t/ build //opencensus/trace | ||
bazel --output_user_root=c:/t/ build %BUILDABLES% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is great overall, but I'd prefer to not have appveyor failing while we work on other stuff.
Please do:
bazel --output_user_root=c:/t/ build //opencensus/trace
REM bazel --output_user_root=c:/t/ build %BUILDABLES%
And we'll incrementally add targets as we get them building on Windows.
(I'm worried if every PR and commit has a red X after it, people will stop paying attention to CI entirely)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. (and //opencensus/stats, which builds once the switch fallthrough errors were suppressed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, awesome! Thanks for that.
tools/appveyor/test.bat
Outdated
|
||
REM TODO: Remove --output_user_root after https://github.com/bazelbuild/bazel/issues/4149 is fixed. | ||
REM bazel --output_user_root=c:/t/ test //opencensus/trace:all | ||
echo TODO: Make all tests pass on Windows. | ||
bazel --output_user_root=c:/t/ test --test_output=errors %TESTS% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
0972804
to
99393a8
Compare
tools/appveyor/test.bat
Outdated
|
||
REM TODO: Remove --output_user_root after https://github.com/bazelbuild/bazel/issues/4149 is fixed. | ||
REM bazel --output_user_root=c:/t/ test //opencensus/trace:all | ||
echo TODO: Make all tests pass on Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: leave the echo as a reminder. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Please update the PR/commit description. |
Full builds and tests are not yet enabled due to lingering compilation errors.
No description provided.