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 "make check" on MinGW. #1496

Merged
merged 2 commits into from
Aug 16, 2022
Merged

Conversation

jkbonfield
Copy link
Contributor

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.)

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.
@jmarshall
Copy link
Member

The test/check target essentially has a dependency on all, except that it spells out what it actually needs explicitly as there's a couple of things in all that it doesn't need.

So what this really is is a missing dependency: test/check should also list a dependency on libhts.$(SHLIB_FLAVOUR) (at least when the test/with-shlib.sh lines want to do something non-trivial) as used on the test/with-shlib.sh lines. Except that using $(SHLIB_FLAVOUR) directly in filenames like that is actually wrong, so those commands already only work on Linux and macOS, where $(SHLIB_FLAVOUR) happens to be exactly the filename extension used.

So probably what this really really wants is these two commands to be pulled out to a separate test-plugins target, that listed as a prerequisite of test/check, and some more variable infrastructure to hold the actual base shared library filename — to be used as a prerequisite of test-plugins and nearby.

@jkbonfield
Copy link
Contributor Author

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 libhts.$(SHLIB_FLAVOUR) for make check (especially given it may not actually be lib, so as with hts-3.dll) when simply sticking all in the dependency line does the job far simpler and is self explanatory.

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.

@daviesrob
Copy link
Member

As far as I can tell, the dependencies are working given that ./configure --enable-plugins causes the shared libhts library to be built along with the plugins on UNIX-y and MinGW platforms (but not MacOS). Without that option you don't get a shared library, and the tests are disabled anyway so skipping them earlier shouldn't be a problem.

However, there's still a problem on MacOS with --enable-plugins:

HTS_PATH=. test/with-shlib.sh test/plugins-dlhts -g ./libhts.dylib
Can't dlopen "./libhts.dylib": dlopen(./libhts.dylib, 10): image not found
make: *** [test] Error 1

As it's test/plugins-dlhts that wants to dlopen("libhts.so") or whatever, it really needs to depend on lib-shared. We then might want to make the dependency of check / test on test/plugins-dlhts optional depending on the state of BUILT_PLUGINS - although we could defer that to future work and just let the main library be built all the time for now.

The other problem with this test is that it uses libhts.$(SHLIB_FLAVOUR) for the dll to load, which doesn't work when there is no lib prefix. We might want to add a SHLIB_NAME variable, set at the same time as SHLIB_FLAVOUR so we know exactly what it's called.

@jkbonfield
Copy link
Contributor Author

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 all as a dependency seems a reasonable solution.

Regarding the libhts. bit, I realised it was the wrong name, but the test is explicitly disabled in the test program for Windows, and it's only windows that doesn't have the lib prefix. In the fullness of time it wants rewriting, but I didn't think that was appropriate for this release so I went with the smallest acceptable change.

With hindsight smaller still would have been what I originally did which was just stick all or lib-shared as a check dependency, and ignore the wrong filename issue (for now).

- "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.
@jkbonfield
Copy link
Contributor Author

Following the meeting discussions, I've lifted over bits of the other branch.

  • "make plugins" now has an explicit dependency on "lib-shared". This was already brought in on linux, but not on some other system types.
  • "make check" now depends on "all" so we get the shared library irrespective of whether it's needed. This is a better workaround than more minutae of dependencies, as knowing we can build everything is a good check in its own right.

@daviesrob daviesrob merged commit 136e4a9 into samtools:develop Aug 16, 2022
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.

3 participants