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

Revert "Suppress some Mac-specific linker warnings." #5362

Merged
merged 1 commit into from
Mar 2, 2017

Conversation

liangfok
Copy link
Contributor

@liangfok liangfok commented Mar 1, 2017

Reverts #5338

It was discovered that this PR resulted in gtest failing to catch exceptions. Specifically, unit tests containing EXPECT_THROW() or EXPECT_ANY_THROW() would fail because the exceptions were not being caught by gTest. Here is an example error:

$ bazel run //drake/systems/primitives:saturation_test
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: Failure at drake/systems/primitives/saturation.cc:23 in Saturation(): condition 'input_size_ > 0' failed.

This change is Reviewable

@liangfok
Copy link
Contributor Author

liangfok commented Mar 1, 2017

@drake-jenkins-bot mac-clang-bazel-nightly-release please

@liangfok
Copy link
Contributor Author

liangfok commented Mar 1, 2017

@drake-jenkins-bot mac-clang-bazel-experimental please.
@drake-jenkins-bot mac-clang-bazel-experimental-everything please.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@liangfok
Copy link
Contributor Author

liangfok commented Mar 1, 2017

+@soonho-tri and +@sammy-tri for both levels of review please. I believe this PR can be merged immediately after the mac-clang-bazel-* CI tests pass.


Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sammy-tri
Copy link
Contributor

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sammy-tri
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@soonho-tri
Copy link
Member

:lgtm:

BTW, I'd like to @david-german-tri to take a look later.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@liangfok
Copy link
Contributor Author

liangfok commented Mar 1, 2017

Thanks. This PR is now waiting on the following CI test to pass before it can be merged.

https://drake-jenkins.csail.mit.edu/job/mac-clang-bazel-experimental-everything/14/


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@BetsyMcPhail
Copy link
Contributor

The mac-clang-bazel-experimental-everything job is still a WIP and depends on a not-yet-merged CI PR. It's status shouldn't hold up this PR.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@liangfok
Copy link
Contributor Author

liangfok commented Mar 1, 2017

Oh, that's great, thanks. What about this build?

https://drake-jenkins.csail.mit.edu/job/mac-clang-bazel-experimental/32/

Should the above CI test hold up this PR from being merged?


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@BetsyMcPhail
Copy link
Contributor

The mac-clang-bazel-experimental is a valid test for this PR (https://drake-jenkins.csail.mit.edu/job/mac-clang-bazel-experimental/31 captured the error)


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@liangfok
Copy link
Contributor Author

liangfok commented Mar 1, 2017

OK thanks. This PR is now blocked on https://drake-jenkins.csail.mit.edu/job/mac-clang-bazel-experimental/32/ passing.


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Passed after 4h42m.

@jwnimmer-tri jwnimmer-tri merged commit 512ed2b into master Mar 2, 2017
@liangfok liangfok deleted the revert-5338-ct branch March 2, 2017 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants