Improve checks for external deps #1246
Conversation
|
Looks like a nice addition to the build systems. Obviously the tests need to pass before this can be merged. Seems like the mingw test needs some special treatment before it can pass. Not sure what's going on with appveyor, maybe just bad connectivity when the checks were run?
Minimum version of what? |
For sqlite3, the previous test checked for a function that seemed to exist since forever, so I don't know if we need any specific minimum version. For Google test, I suppose the minimum can be whatever is bundled right now. The AppVeyor fail looks unrelated, but I will look into the mingw issue. |
|
Minimum version for sqlite is 3.7 (see RFC3) Yeah, the bundled version of googletest is fine. |
|
The mingw target emits "WARNING: unrecognized options: --with-sqlite3_include, --with-sqlite3_ldflags" at PROJ configure time. Likely it would need a PKG_CONFIG_PATH= override instead |
QuLogic
force-pushed
the
external-deps
branch
2 times, most recently
from
b3f5e20
to
ca7f870
Compare
|
OK, I added the minimum versions in the checks, and for mingw, installed sqlite3 into a standard directory that pkg-config could find it. |
QuLogic
force-pushed
the
external-deps
branch
from
ca7f870
to
49123c6
Compare
|
Thanks for taking care of this @QuLogic! |
| if (USE_EXTERNAL_GTEST) | ||
|
|
||
| message(STATUS "Using external GTest") | ||
| find_package(GTest 1.8.0) |
There was a problem hiding this comment.
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 the else 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:
- find_package and prefix corrections
- googletest 1.8.1
- ph_phi2_test correction
There was a problem hiding this comment.
I can prepare a PR with these changes if desired
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.
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.
Have you tried it? I just tried prefixing them and it doesn't understand it (with GTest 1.8.1.)
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.
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.
Use pkgconfig to find sqlite3 (only in autotools, as I'm not sure how to get it right for CMake.) This automatically adds override variables, so it really simplifies what needs to be written by hand.
Then add the option of building against external GTest. This is for packagers who want to build against system copies. I was able to do this one for both autotools and cmake.
Both of these could specify a minimum version, but I'm not sure what they should be.