-
Notifications
You must be signed in to change notification settings - Fork 465
Windows compilation. #531
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
Windows compilation. #531
Changes from 2 commits
3dc3584
ace007f
6827df2
b2ceb11
6c5b182
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # version format. | ||
| # you can use {branch} name in version format too | ||
| # version: 1.0.{build}-{branch} | ||
| version: 'vers.{build}' | ||
|
|
||
| # branches to build | ||
| branches: | ||
| # Whitelist | ||
| only: | ||
| - develop | ||
|
|
||
| # Blacklist | ||
| except: | ||
| - gh-pages | ||
|
|
||
| # Do not build on tags (GitHub and BitBucket) | ||
| skip_tags: true | ||
|
|
||
| # Skipping commits affecting specific files (GitHub only). More details here: /docs/appveyor-yml | ||
| #skip_commits: | ||
| # files: | ||
| # - docs/* | ||
| # - '**/*.html' | ||
|
|
||
| # We use Mingw/Msys, so use pacman for installs | ||
| install: | ||
| - set HOME=. | ||
| - set MSYSTEM=MINGW64 | ||
| - set PATH=C:/msys64/usr/bin;C:/msys64/mingw64/bin;%PATH% | ||
| - set MINGWPREFIX=x86_64-w64-mingw32 | ||
| - "sh -lc \"pacman -S --noconfirm --needed base-devel mingw-w64-x86_64-toolchain mingw-w64-x86_64-zlib mingw-w64-x86_64-bzip2 mingw-w64-x86_64-xz mingw-w64-x86_64-curl\"" | ||
|
|
||
| build_script: | ||
| - set HOME=. | ||
| - set MSYSTEM=MINGW64 | ||
| - set PATH=C:/msys64/usr/bin;C:/msys64/mingw64/bin;%PATH% | ||
| - "sh -lc \"aclocal && autoheader && autoconf && ./configure && make -j2\"" | ||
|
|
||
| #build_script: | ||
| # - make | ||
|
|
||
| test_script: | ||
| - "sh -lc \"make test\"" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| *.o | ||
| *.pico | ||
| *.obj | ||
| *.dSYM | ||
| *.exe | ||
| *.dll | ||
| *.pc.tmp | ||
| *-uninstalled.pc | ||
| /version.h | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,6 +138,7 @@ print-version: | |
|
|
||
|
|
||
| LIBHTS_OBJS = \ | ||
| hts_os.o\ | ||
| kfunc.o \ | ||
| knetfile.o \ | ||
| kstring.o \ | ||
|
|
@@ -210,6 +211,8 @@ config.h: | |
| echo '/* Default config.h generated by Makefile */' > $@ | ||
| echo '#define HAVE_LIBBZ2 1' >> $@ | ||
| echo '#define HAVE_LIBLZMA 1' >> $@ | ||
| echo '#define HAVE_FSEEKO 1' >> $@ | ||
| echo '#define HAVE_DRAND48 1' >> $@ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than adding infrastructure for these functions, wouldn't it be better to see where they are used: fseeko (it isn't); drand48 (just in ks_shuffle, which could use native alternatives)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fseeko is used and drand48 is used by more than ks_shuffle. Possibly we could rewrite the code that uses both, but as these are POSIX functions the intent is simply to put OS-specific deficiencies in one single place so we can maintain our position of desiring a POSIX compatible compilation. It also makes the PR much simpler, and safer, as it doesn't contain a lot of rewriting of code just to make it run on Windows.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only place that needs to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The one in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not, but please say what you mean then. You started by saying it's unused, then by saying it's used by code which isn't used, and now by saying it's used in code that could be rewritten. The conversation would be shorter if you just said to start with that it could be avoided by replacing zfio with calls to bgzf - which I fully agree with. However this just wasted all our time instead.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO I was perfectly clear for people willing to read between the lines at all; i.e., approaching the conversation with a view of ”what is the reason behind that particular statement” rather than “that statement is wrong”. Anyway: PR #553 fixes a minor bug too. Win/win. |
||
|
|
||
| # And similarly for htslib.pc.tmp ("pkg-config template"). No dependency | ||
| # on htslib.pc.in listed, as if that file is newer the usual way to regenerate | ||
|
|
@@ -237,6 +240,9 @@ lib-shared: libhts.dylib | |
| else ifeq "$(findstring CYGWIN,$(PLATFORM))" "CYGWIN" | ||
| SHLIB_FLAVOUR = cygdll | ||
| lib-shared: cyghts-$(LIBHTS_SOVERSION).dll | ||
| else ifeq "$(findstring MSYS,$(PLATFORM))" "MSYS" | ||
| SHLIB_FLAVOUR = dll | ||
| lib-shared: hts-$(LIBHTS_SOVERSION).dll | ||
| else | ||
| SHLIB_FLAVOUR = so | ||
| lib-shared: libhts.so | ||
|
|
@@ -278,6 +284,9 @@ libhts.dylib: $(LIBHTS_OBJS) | |
| cyghts-$(LIBHTS_SOVERSION).dll: $(LIBHTS_OBJS) | ||
| $(CC) -shared -Wl,--out-implib=libhts.dll.a -Wl,--export-all-symbols -Wl,--enable-auto-import $(LDFLAGS) -o $@ -Wl,--whole-archive $(LIBHTS_OBJS) -Wl,--no-whole-archive $(LIBS) -lpthread | ||
|
|
||
| hts-$(LIBHTS_SOVERSION).dll: $(LIBHTS_OBJS) | ||
| $(CC) -shared -Wl,--out-implib=hts.dll.a -Wl,--export-all-symbols -Wl,--enable-auto-import $(LDFLAGS) -o $@ -Wl,--whole-archive $(LIBHTS_OBJS) -Wl,--no-whole-archive $(LIBS) -lpthread | ||
|
|
||
|
|
||
| .pico.so: | ||
| $(CC) -shared -Wl,-E $(LDFLAGS) -o $@ $< $(LIBS) -lpthread | ||
|
|
@@ -288,6 +297,9 @@ cyghts-$(LIBHTS_SOVERSION).dll: $(LIBHTS_OBJS) | |
| .o.cygdll: | ||
| $(CC) -shared $(LDFLAGS) -o $@ $< libhts.dll.a $(LIBS) | ||
|
|
||
| .o.dll: | ||
| $(CC) -shared $(LDFLAGS) -o $@ $< hts.dll.a $(LIBS) | ||
|
|
||
|
|
||
| bgzf.o bgzf.pico: bgzf.c config.h $(htslib_hts_h) $(htslib_bgzf_h) $(htslib_hfile_h) $(htslib_thread_pool_h) cram/pooled_alloc.h $(htslib_khash_h) | ||
| errmod.o errmod.pico: errmod.c config.h $(htslib_hts_h) $(htslib_ksort_h) | ||
|
|
@@ -352,6 +364,9 @@ tabix.o: tabix.c config.h $(htslib_tbx_h) $(htslib_sam_h) $(htslib_vcf_h) $(htsl | |
|
|
||
| # For tests that might use it, set $REF_PATH explicitly to use only reference | ||
| # areas within the test suite (or set it to ':' to use no reference areas). | ||
| # | ||
| # If using MSYS, avoid poor shell expansion via: | ||
| # MSYS2_ARG_CONV_EXCL="*" make check | ||
| check test: $(BUILT_PROGRAMS) $(BUILT_TEST_PROGRAMS) | ||
| test/hts_endian | ||
| test/fieldarith test/fieldarith.sam | ||
|
|
@@ -360,7 +375,7 @@ check test: $(BUILT_PROGRAMS) $(BUILT_TEST_PROGRAMS) | |
| cd test/tabix && ./test-tabix.sh tabix.tst | ||
| REF_PATH=: test/sam test/ce.fa test/faidx.fa | ||
| test/test-regidx | ||
| cd test && REF_PATH=: ./test.pl | ||
| cd test && REF_PATH=: ./test.pl $${TEST_OPTS:-} | ||
|
|
||
| test/hts_endian: test/hts_endian.o | ||
| $(CC) $(LDFLAGS) -o $@ test/hts_endian.o $(LIBS) | ||
|
|
@@ -450,6 +465,10 @@ install-cygdll: cyghts-$(LIBHTS_SOVERSION).dll installdirs | |
| $(INSTALL_PROGRAM) cyghts-$(LIBHTS_SOVERSION).dll $(DESTDIR)$(bindir)/cyghts-$(LIBHTS_SOVERSION).dll | ||
| $(INSTALL_PROGRAM) libhts.dll.a $(DESTDIR)$(libdir)/libhts.dll.a | ||
|
|
||
| install-dll: hts-$(LIBHTS_SOVERSION).dll installdirs | ||
| $(INSTALL_PROGRAM) hts-$(LIBHTS_SOVERSION).dll $(DESTDIR)$(bindir)/hts-$(LIBHTS_SOVERSION).dll | ||
| $(INSTALL_PROGRAM) hts.dll.a $(DESTDIR)$(libdir)/hts.dll.a | ||
|
|
||
| install-dylib: libhts.dylib installdirs | ||
| $(INSTALL_PROGRAM) libhts.dylib $(DESTDIR)$(libdir)/libhts.$(PACKAGE_VERSION).dylib | ||
| ln -sf libhts.$(PACKAGE_VERSION).dylib $(DESTDIR)$(libdir)/libhts.dylib | ||
|
|
@@ -486,6 +505,9 @@ clean-so: | |
| clean-cygdll: | ||
| -rm -f cyghts-*.dll libhts.dll.a | ||
|
|
||
| clean-dll: | ||
| -rm -f hts-*.dll hts.dll.a | ||
|
|
||
| clean-dylib: | ||
| -rm -f libhts.dylib libhts.*.dylib | ||
|
|
||
|
|
@@ -513,4 +535,5 @@ force: | |
| .PHONY: tags test testclean | ||
| .PHONY: clean-so install-so | ||
| .PHONY: clean-cygdll install-cygdll | ||
| .PHONY: clean-dll install-dll | ||
| .PHONY: clean-dylib install-dylib | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,11 @@ | |
| #include "htslib/bgzf.h" | ||
| #include "htslib/hts.h" | ||
|
|
||
| #ifdef _WIN32 | ||
| # define WIN32_LEAN_AND_MEAN | ||
| # include <windows.h> | ||
| #endif | ||
|
|
||
| static const int WINDOW_SIZE = 64 * 1024; | ||
|
|
||
| static void error(const char *format, ...) | ||
|
|
@@ -198,6 +203,9 @@ int main(int argc, char **argv) | |
|
|
||
| if ( index ) bgzf_index_build_init(fp); | ||
| buffer = malloc(WINDOW_SIZE); | ||
| #ifdef _WIN32 | ||
| _setmode(f_src, O_BINARY); | ||
| #endif | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the decision here is to have bgzip open everything as binary (cf second paragraph of this comment), wouldn't it be better to just use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we decided that hopen musn't do the The change was made for a reason (it fixes bugs). If your view is that bgzip must NOT open everything as binary then we need to change the tool further still, adding a command line argument to tell it whether or not to treat stdin as text or binary. Yet more complications.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reread my comment. It's talking about changing bgzip.c to use I changed bgzip's BGZF files from
Apparently you have now analysed the effects, though that wasn't apparent from the commit messages. Grand; and I guess it gives So the maintenance question is about the risks of a change that affects all platforms while regularising and simplifying the code, versus one that adds the maintenance burden of |
||
| if (rebgzip){ | ||
| if ( bgzf_index_load(fp, index_fname, NULL) < 0 ) error("Could not load index: %s.gzi\n", argv[optind]); | ||
|
|
||
|
|
@@ -319,13 +327,21 @@ int main(int argc, char **argv) | |
| if ( bgzf_index_load(fp, argv[optind], ".gzi") < 0 ) error("Could not load index: %s.gzi\n", argv[optind]); | ||
| if ( bgzf_useek(fp, start, SEEK_SET) < 0 ) error("Could not seek to %d-th (uncompressd) byte\n", start); | ||
| } | ||
| #ifdef _WIN32 | ||
| _setmode(f_dst, O_BINARY); | ||
| #endif | ||
| while (1) { | ||
| if (end < 0) c = bgzf_read(fp, buffer, WINDOW_SIZE); | ||
| else c = bgzf_read(fp, buffer, (end - start > WINDOW_SIZE)? WINDOW_SIZE:(end - start)); | ||
| if (c == 0) break; | ||
| if (c < 0) error("Could not read %d bytes: Error %d\n", (end - start > WINDOW_SIZE)? WINDOW_SIZE:(end - start), fp->errcode); | ||
| start += c; | ||
| if ( write(f_dst, buffer, c) != c ) error("Could not write %d bytes\n", c); | ||
| if ( write(f_dst, buffer, c) != c ) { | ||
| #ifdef _WIN32 | ||
| if (GetLastError() != ERROR_NO_DATA) | ||
| #endif | ||
| error("Could not write %d bytes\n", c); | ||
| } | ||
| if (end >= 0 && start >= end) break; | ||
| } | ||
| free(buffer); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,7 @@ AC_ARG_ENABLE([gcs], | |
| [], [enable_gcs=check]) | ||
|
|
||
| AC_SYS_LARGEFILE | ||
| AC_FUNC_FSEEKO | ||
|
|
||
| AC_ARG_ENABLE([libcurl], | ||
| [AS_HELP_STRING([--enable-libcurl], | ||
|
|
@@ -112,8 +113,8 @@ AC_ARG_ENABLE([s3], | |
| [support Amazon AWS S3 URLs])], | ||
| [], [enable_s3=check]) | ||
|
|
||
| AC_MSG_CHECKING([shared library type]) | ||
| test -n "$host_alias" || host_alias=unknown-`uname -s` | ||
| AC_MSG_CHECKING([shared library type for $host_alias]) | ||
| case $host_alias in | ||
| *-cygwin* | *-CYGWIN*) | ||
| host_result="Cygwin DLL" | ||
|
|
@@ -125,6 +126,15 @@ case $host_alias in | |
| PLATFORM=Darwin | ||
| PLUGIN_EXT=.bundle | ||
| ;; | ||
| *-msys* | *-MSYS* | *-mingw* | *-MINGW*) | ||
| host_result="MSYS dll" | ||
| PLATFORM=MSYS | ||
| PLUGIN_EXT=.dll | ||
| # This also sets __USE_MINGW_ANSI_STDIO which in turn makes PRId64, | ||
| # %lld and %z printf formats work. It also enforces the snprintf to | ||
| # be C99 compliant so it returns the correct values (in kstring.c). | ||
| CPPFLAGS="$CPPCFLAGS -D_POSIX_C_SOURCE=600" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change |
||
| ;; | ||
| *) | ||
| host_result="plain .so" | ||
| PLATFORM=default | ||
|
|
@@ -136,7 +146,7 @@ AC_SUBST([PLATFORM]) | |
|
|
||
| dnl FIXME This pulls in dozens of standard header checks | ||
| AC_FUNC_MMAP | ||
| AC_CHECK_FUNCS(gmtime_r) | ||
| AC_CHECK_FUNCS([gmtime_r fsync drand48]) | ||
|
|
||
| # Darwin has a dubious fdatasync() symbol, but no declaration in <unistd.h> | ||
| AC_CHECK_DECL([fdatasync(int)], [AC_CHECK_FUNCS(fdatasync)]) | ||
|
|
@@ -183,9 +193,11 @@ FAILED. This error must be resolved in order to build HTSlib successfully.]) | |
| fi | ||
|
|
||
| dnl connect() etc. fns are in libc on linux, but libsocket on illumos/Solaris | ||
| libsocket=unneeded | ||
| AC_SEARCH_LIBS(connect, socket, [libsocket=needed], []) | ||
|
|
||
| AC_SEARCH_LIBS([recv], [socket ws2_32], [ | ||
| if test "$ac_cv_search_recv" != "none required" | ||
| then | ||
| static_LIBS="$static_LIBS $ac_cv_search_recv" | ||
| fi], [AC_MSG_ERROR([unable to find the recv() function])]) | ||
|
|
||
| if test "$enable_bz2" != no; then | ||
| bz2_devel=ok | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,7 +192,7 @@ typedef struct cram_file_def { | |
| struct cram_slice; | ||
|
|
||
| enum cram_block_method { | ||
| ERROR = -1, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change and its parallel MF_APPEND are caused by a clash with Windows.h. The workaround used here certainly works, but I'd recommend a less intrusive one instead: define NOUSER and NOGDI when compiling libhts. That shouldn't interfere with anything on any other platforms.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noted that the affected header files aren't part of the public API. So perhaps less of an issue than I originally thought.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. I figured it's probably a good move anyway to minimally name-space that ERROR enum as the potential for clashes on other systems will be reduced. |
||
| BM_ERROR = -1, | ||
| RAW = 0, | ||
| GZIP = 1, | ||
| BZIP2 = 2, | ||
|
|
||
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.
Other entries here are clearly sorted by klib, then alphabetically, then subdirectories.
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.
Agreed.