-
Notifications
You must be signed in to change notification settings - Fork 446
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 "make check" on MinGW. #1496
Conversation
If we haven't previously done a "make" or "make all", the .dll file won't have been created. The test/with-shlib.sh script will then fail as the "cp -p ../../hts-*.dll ." command fails. We no longer even attempt to execute this test when plugins don't exist (which they won't on Windows unless we've installed the dlfcn package). Note the analogous non-mingw commands are "ln -s ../../libhts.so.* ." and similar, which don't fail in the $? sense, but do fail with regards to produced a bogus target containing a wild-card pattern. This implies the error checking in this script is broken for all platforms bar MinGW, but given the very limited use for this one test case it's an irrelevance.
The So what this really is is a missing dependency: So probably what this really really wants is these two commands to be pulled out to a separate |
I initially assumed it was missing a dependency on all, but after adding it I realised it doubled the compilation time everywhere as it's building both libraries. Arguably that's correct - it is test after all - but if we're not using the shared library I had assumed this nuance was a deliberate one. Hence my fix. I'm happy if we feel it should do a "make all" as part of check. I don't see why we'd want to go to the bother of I think there is more work to do here, in particular figuring out why all bar 2 plugins work on Windows MINGW. It's clearly capable of using dlopen and dlsym, but I haven't worked out what's exceptional about s3 and gcs yet. I don't propose to do that for this release. Given we're not planning to get plugins working immediately on Windows, I figured the simplest fix was basically to elide the offending tests for now as it's short and sweet, and then work out how best to solve this later on. |
As far as I can tell, the dependencies are working given that However, there's still a problem on MacOS with
As it's The other problem with this test is that it uses |
That's what my other branch was doing, by conditionally modifying the dependencies. However I think it's reasonable to say that "make check" perhaps ought to check everything can be built (if it's not been built already), so just having Regarding the With hindsight smaller still would have been what I originally did which was just stick |
- "make plugins" now also explicitly builds the shared library. This wasn't universally true before, depending on system type. - "make check" and "make test" now has "all" as a dependency. While strictly not everything is a dependency of running the tests, ensuring all the code compiles can be viewed as one of the tests to perform (albeit only executed the first time due to makefile dependency checking rules). This also aids some platforms where building the shared libary was not a prerequisite, causing the plugins tests to fail if not already built.
Following the meeting discussions, I've lifted over bits of the other branch.
|
If we haven't previously done a "make" or "make all", the .dll file
won't have been created. The test/with-shlib.sh script will then fail
as the "cp -p ../../hts-*.dll ." command fails. We no longer even
attempt to execute this test when plugins don't exist (which they
won't on Windows unless we've installed the dlfcn package).
Note the analogous non-mingw commands are "ln -s ../../libhts.so.* ."
and similar, which don't fail in the $? sense, but do fail with
regards to produced a bogus target containing a wild-card pattern.
This implies the error checking in this script is broken for all
platforms bar MinGW, but given the very limited use for this one test
case it's an irrelevance.
(See also the abandoned https://github.com/jkbonfield/htslib/tree/mingw_make_check which adds code so
--enable-plugins
builds on MinGW. Sadly it doesn't work for gs: and s3:, with cryptic errors, so delaying that to another time.)