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 FindLibraryTest by making it platform-dependent #7632

Merged

Conversation

soonho-tri
Copy link
Member

@soonho-tri soonho-tri commented Dec 16, 2017

The original code was added by #7439.
See https://drake-jenkins.csail.mit.edu/view/Continuous%20Production/job/mac-sierra-clang-bazel-continuous-release/271/consoleText.

This PR merely changes the test to make CI happy. @fbudin69500, please check if the original test failure indicates an issue in LoadedLibraryPath.

/cc @jamiesnape, @sammy-tri, @jwnimmer-tri (reviewers of #7439)
/cc @ggould-tri (buildcop)


This change is Reviewable

@soonho-tri
Copy link
Member Author

@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please

@soonho-tri
Copy link
Member Author

+@fbudin69500 for feature-review, please.


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


Comments from Reviewable

@fbudin69500
Copy link

:lgtm:


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


Comments from Reviewable

@fbudin69500
Copy link

Thanks for solving this issue. I'll double check that there is not a problem in the function that is tested, but this seems like a very simple fix that makes sense. The function LoadedLibraryPath() is typically used to check where libdrake.so is, but in this simple test, I didn't want to add complex dependencies to libdrake.so, so I settled in using libgflags.so which was used by this test. I forgot to verify that its name was the same on all platforms...
As a side note, example tests verify that this function works with libdrake.so, so I am fairly confident that that function works.


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


Comments from Reviewable

@soonho-tri
Copy link
Member Author

+@EricCousineau-TRI for platform-review, please.


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


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

  // loaded by the process.
  optional<string> library_path =
    LoadedLibraryPath("libexternal_Scom_Ugithub_Ugflags_Ugflags_Slibgflags.so");

BTW It was a little hard for me to figure out at what point in the chain libglfags is effectively linked in as a shared rather than static library.

Could you either (a) manually create a test lib_is_real.so (using drake_cc_binary) to ensure that this test is not dependent on how we link libgflags (as it could change with something like #7616), or (b) document how libgflags is being dynamically linked, and under what conditions it would not be dynamically linked?


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW It was a little hard for me to figure out at what point in the chain libglfags is effectively linked in as a shared rather than static library.

Could you either (a) manually create a test lib_is_real.so (using drake_cc_binary) to ensure that this test is not dependent on how we link libgflags (as it could change with something like #7616), or (b) document how libgflags is being dynamically linked, and under what conditions it would not be dynamically linked?

Sorry, promoting this to blocking rather than BTW.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

Sorry, promoting this to blocking rather than BTW.

Can that be a follow up so that we get the Mac builds blue again?


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor

:lgtm:


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Can that be a follow up so that we get the Mac builds blue again?

Yup.


Comments from Reviewable

@soonho-tri
Copy link
Member Author

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Yup.

@fbudin69500 , could you open an issue to keep track of the items that @EricCousineau-TRI suggested?


Comments from Reviewable

@soonho-tri soonho-tri merged commit 2b581ab into RobotLocomotion:master Dec 18, 2017
@soonho-tri soonho-tri deleted the pr-fix-find_loaded_library_test branch December 18, 2017 15:59
@EricCousineau-TRI
Copy link
Contributor

common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

@fbudin69500 , could you open an issue to keep track of the items that @EricCousineau-TRI suggested?

I'll go ahead and do this; it should take about 10min (fingers crossed).


Comments from Reviewable

@soonho-tri
Copy link
Member Author

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


common/test/find_loaded_library_test.cc, line 16 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

I'll go ahead and do this; it should take about 10min (fingers crossed).

Thank you. FYI, just merged this PR into master. I expect the mac CI will turn to blue!


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants