Skip to content

Conversation

@jkbonfield
Copy link
Contributor

MINGW x64 works fine, but UCRT-x64 fails one of the setGT tests. Specifically, -n c:././. gets silently rewritten before main() so the argv element contains -n c;.\.\. instead. This is a pain, and arguably we should provide a less problematic CLI alternative for this as c: was always going to trip up Windows boxes.

However for now we can work around it by not having it as a separate argument and letting getopt do the tokenisation of argv into option and optarg for us by removing the space.

This doesn't fix the bug obviously, but it makes it pass tests if using the UCRT environment.

MINGW x64 works fine, but UCRT-x64 fails one of the setGT tests.
Specifically, `-n c:././.` gets silently rewritten before main() so
the argv element contains `-n c;.\.\.` instead.  This is a pain, and
arguably we should provide a less problematic CLI alternative for this
as c: was always going to trip up Windows boxes.

However for now we can work around it by not having it as a separate
argument and letting getopt do the tokenisation of argv into option
and optarg for us by removing the space.

This doesn't fix the bug obviously, but it makes it pass tests if
using the UCRT environment.
@pd3
Copy link
Member

pd3 commented May 28, 2025

Wow, this is ugly. If the argument were quoted or escaped somehow, something like -n 'c:././.', or -n c\:././., would it stop being rewritten?

@jkbonfield
Copy link
Contributor Author

Sadly not, no. It's just a particularly dumb thing in the Microsoft runtime equivalent of crt0: the bit that happens before main() gets called with the tokenisation of the command line. That makes it really hard to work around in code, unless you want to explicitly recognise both semicolons and colons and both forward and backwards slashes.

My fix for now was just to make the test pass so I don't trip over it again in the future, but long term we need to figure out how best to address such brokenness.

@pd3 pd3 merged commit bf5fc6e into samtools:develop May 29, 2025
8 checks passed
@pd3
Copy link
Member

pd3 commented May 29, 2025

Thank you

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.

2 participants