-
Notifications
You must be signed in to change notification settings - Fork 459
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 all 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 |
---|---|---|
|
@@ -192,7 +192,7 @@ typedef struct cram_file_def { | |
struct cram_slice; | ||
|
||
enum cram_block_method { | ||
ERROR = -1, | ||
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. 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. 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.
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
hopen()
rather than sprinkling _WIN32 platform crap all over the code?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.
I thought we decided that hopen musn't do the
O_BINARY
setmode thing itself, indeed I recall you squashing a PR on that very reason. Plus again we'd be changing bgzip on Unix in order to make it run on Windows, which comes with potential bugs and risks. I'm happier with a windows-only change personally.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.
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.
Reread my comment. It's talking about changing bgzip.c to use
hopen
instead ofopen
. It does not propose altering the implementation ofhopen
itself.I changed bgzip's BGZF files from
open
to (effectively)hopen
in b8204c1 during the PR you mention, which is linked in my previous comment. I would have rather liked to change its other files too as it simplifies and regularises the code, but I had not analysed the effects that would have on Windows, as I noted at the time.Apparently you have now analysed the effects, though that wasn't apparent from the commit messages. Grand; and I guess it gives
bgzip -i
a chance to have the right offsets.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
#ifdef
spaghetti.