-
Notifications
You must be signed in to change notification settings - Fork 792
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
Improve checks for external deps #1246
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
3224932
Use pkgconfig to find sqlite3.
QuLogic 561a663
Allow building against external GTest with autotools.
QuLogic 7c9e460
Allow building against external GTest with CMake.
QuLogic 49123c6
CI: Install cross-compiled sqlite3 to cross directory.
QuLogic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How is this supposed to work? The Gtest configuration file exports prefixed targets, e.g.
GTest::gtest
. But the targets below link unprefixed targets, e.g.gtest
. Thus the build rules do not pick up the include directories etc. carried by the target.So IMO the build rules need to use the prefix, and for consistency, a
find_package
with the explicit internal path should be added to theelse
branch (internal GTest).BTW it wouldn't hurt to declare this
REQUIRED
.BTW 1.8.0 will not work with MinGW. 1.8.1 is fixed, google/googletest#721.
BTW later in this file there is a test commented as "ph_phi2_test not compatible of a .dll build". The surrounding tests needs to use WIN32 instead of MSVC, because its not compatible with MinGW, too.
I can prepare a PR with these changes if desired:
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 do. But be aware that I am preparing a release candidate tomorrow so you need to work fast.
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.
Have you tried it? I just tried prefixing them and it doesn't understand it (with GTest 1.8.1.)
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.
Yes, I tried with
USE_EXTERNAL_GTEST=ON
and CMake 3.11 or so.Did you check the command line how the tests are compiled? I couldn't spot the include directories before the change. I guess it works for you because you have GTest in the compiler's standard directories. But these are not always the right ones for cmake, depending on
CMAKE_FIND_...
configuration.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.
No, I mean CMake (3.12.1) doesn't configure. It's true that everything here is in a standard directory though, for the working build.
If you open a PR, I can try out what you mean.
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.
Created #1265.