Provide a definition of ssize_t when compiling with MSVC [more robust alternative to #1375] #1377
Conversation
|
This addresses the For compiling the HTSlib source files themselves, the best approach to addressing this This would usually be done in configure, but people wanting to compile HTSlib with MSVC are probably not in a position to be running a Unix shell script! An alternative would be a pre-written bespoke config-win.h, but really choosing the best way to proceed here (if at all) requires input from someone who actually uses MSVC. (There is |
|
Thanks. It's a relatively small change, but I'll delay reviewing and merging for now until I've tested MSVC for real. Basically I want to know if this is the only issue it finds. If the entire library can be linked against and public headers |
|
Happy to help here if I can, the |
Define `ssize_t` within the public headers that use it, enabling them to be compiled with MSVC, which does not provide this type definition in its <sys/types.h> or any other headers. Having `ssize_t` as a macro may confuse other headers, so be sure to do this only after all other #includes and to undefine it again at the end of each header. User code or other libraries may also have workarounds for this issue, so checking its not already #defined and removing our `ssize_t` macro afterwards also avoids conflicting with other workarounds. Under MSVC, including <stdint.h>/<string.h>/etc also causes <vcruntime.h> to be included, which defines `size_t` and `intptr_t` as unsigned/signed versions of the same type. So we define `ssize_t` as that, if it is not already defined (by some other workaround) and if `_INTPTR_T_DEFINED` has indeed been defined by <vcruntime.h> (to avoid confusing error messages). We also check `_SSIZE_T_DEFINED`, which MSVC will likely define if it's ever fixed to supply this typedef. Also define it (permanently) in <htslib/hts_os.h>, unless suppressed by defining `HTS_NO_SSIZE_T` before including the header.
|
Now also checks @BickfordA: It would be useful if you would test building under MSVC using this PR instead of your |
|
This works for me! I no longer need to add anything to the I do still need to add some definitions to my config.h file to cover some of the places not in that commit: If I don't add that to the config.h i get these errors: |
|
I think @jmarshall's changes are to the public headers, meaning when using a pre-built htslib from within MSVC. Your changes are for compiling htslib itself from MSVC. That's an entirely new level of pain. I don't think it's workable without a lot more fiddling with the Makefile (for generating all that extra stuff in config.h) and/or your cmake config (although surely that's also well out of a limb as far as most MSVC users go - they'd want a project file), plus lots of extra work for import libraries and the like. Given you're already willing to use a bunch of unix tools, headers and libraries in order to compile, what is your rationale for using the MSVC compiler specifically rather than using msys2/mingw? |
It's been a good 20 years since I had to use MSVC in anger, and I'm now remembering why I expunged all I learnt about it back then. Actually I'm starting to think using mingw libs from msvc is just plain not possible! I'm sure I've seen it done before, but I can't get it to work. I'd still like to know your thinking though too. I've been trying to get MSVC to link against a mingw prebuilt library, but it's challenging. The steps I've tried to take are:
All of that works and produces me a binary. However that binary then fails on first malloc (I think). Eg an htsfile equivalent built that way displays usage, but cannot parse a BAM file: The other issue is that it would require a ghastly number of ancillary dlls: That would be a rediculous amount of cruft to have to ship with your dll. I don't know why so many, but my money would be on libcurl. (I can't see any other reason for thins like zstd and sqlite.) We could produce a cut down libary, but that then invites all sorts of bug reports about why it can't do X, Y and Z, so I reject that idea. The alternative is to make a static .lib instead, but static libs in Unix at least are normally just the .o files and the person doing the linking then has to add all those dependencies themselves. So I'm concluding the MSVC just isn't sufficiently compatible with mingw to be viable. That sort of renders htslib as useless, unless you're also another mingw user (which realistically means you're a unix user and it's more likely you'll be using unix direct or via WSL within windows, rendering a native windows binary copy of the library somewhat moot). I'd love to hear otherwise though! So the alternative is to fully support MSVC directly. That's a bit of a pain, and I'm not sure any of us really have the energy to actively maintain such a thing ourselves, not being Microsoft developers. If there was a PR showing all the necessary changes that are needed to get the package to cleanly build under MSVC then we could review what it entails, but my gut instinct here having tried it is it's too much given it'd require shipping lots of third party things such as a pthreads and getopt implementations. I would recommend instead WIndows users learn to love WSL instead. |
|
I take that last statement back. It's been so long since I installed MINGW that I forgot the nuances. Thanks to @daviesrob for reminding me that both the MSYS environment matters and also the package names (pacman doesn't use the environment to work out the repository to install from). Yes, I'll document all this in INSTALL. With So this does go back to the main thrust of this PR. However it needed a lot more than just ssize_t to get it to work. I need to work out what's required yet by my test program (ie test_view) and what was in the library headers only. |
|
I can confirm that this PR works, but I can also confirm that So while this PR simplifies things a bit, it's not the only viable method. We could instead simply document the lack of ssize_t and recommend apropriate compiler variables are set to compensate for lack of POSIX compliance. I'm OK with both, although I note this may accidentally lead to breakage in the future while we have to remember to add this boilerplate any time we use ssize_t in a new file. Ie that's a policy decision I'll punt upwards. :) |
|
Anyway, the salient policy difference is between MSVC users needing to do nothing special to include HTSlib headers, just as users on other platforms do, and MSVC users needing to do one small special thing first, whether that is a Despite having proposed this PR, I am in two minds as to whether ten lines of boilerplate in each header that uses The alternative would be to apply just the hts_os.h change, report an “add |
|
I did read your commit message, but I thought I'd experimentally try it without an explicit include just to see how often it was a problem in practice. I'll also add, as alluded to elsewhere (not sure which issue now), most compilers have the option to do a pre-include forcing it before the compilation. Sure enough MSVC is no exception: https://docs.microsoft.com/en-us/cpp/build/reference/fi-name-forced-include-file?view=msvc-170 What this means is that basically the choice here is between documented appropriate CFLAGS that may be useful when using MSVC against our public headers, vs modifying the headers themselves. I can see both sides of this argument and it's unclear what's most appropriate. |
|
We discussed it in our weekly meeting and the conclusion was this is probably the best of the various choices. Thanks. |
PR #1375 proposes unconditionally typedeffing
ssize_ton MSVC in htslib/hts_defs.h, but as discussed in #1025 (comment) this is fragile if user code or other header files#includedalso include their own workarounds for thisssize_tissue. (It would be reminiscent of the olden days when every C library had its own bool type!)A robust alternative is to
#define ssize_twithin each affected HTSlib header only, undoing it at the end of the header:Define
ssize_twithin the public headers that use it, enabling them to be compiled with MSVC, which does not provide this type definition in its <sys/types.h> or any other headers.Having
ssize_tas a macro may confuse other headers, so be sure to do this only after all other #includes and to undefine it again at the end of each header. User code or other libraries may also have workarounds for this issue, so checking its not already #defined and removing ourssize_tmacro afterwards also avoids conflicting with other workarounds.Under MSVC, including <stdint.h>/<string.h>/etc also causes <vcruntime.h> to be included, which defines
size_tandintptr_tas unsigned/signed versions of the same type. So we definessize_tas the latter, if it is not already defined (by some other workaround) and if_INTPTR_T_DEFINEDhas indeed been defined by <vcruntime.h> (to avoid confusing error messages).(I assume that Cygwin and MinGW compilations always use GCC. If users might use Cygwin or MinGW headers with the MSVC compiler, the
#ifwill need some Cygwin and MinGW guards too.)Also define it (permanently) in <htslib/hts_os.h>, unless suppressed by defining
HTS_NO_SSIZE_Tbefore including the header.At the time of #1025, this
ssize_tproblem was the only thing preventing public HTSlib headers from being easily included from user code compiled by MSVC. I have not checked to see if this is still the case in 2022.This workaround is admittedly somewhat voluminous!
An alternative would be to define a
hts_ssize_ttypedef asssize_t/SSIZE_Tin <htslib/hts_defs.h> and use that instead ofssize_tin function signatures in other public HTSlib headers. IMHO this would be a very unfortunate approach, as it would mislead API users into thinking they had to use that type name in their own code instead of the familiar POSIX standardssize_ttype name.