Enable Windows Compatibility #9294
Conversation
This commit fixes Windows path compatibility issues by: 1. Using File.getAbsolutePath() for temp directory paths 2. Adding null check before deleting temp file 3. Using File.separator instead of hardcoded "/" in test paths Changes: - CommandLineProgram.java: Fix tmpDir path handling for Windows - Test files: Replace hardcoded "/" with File.separator for cross-platform compatibility This is a cleaned version of commit 476eebda8 containing only the actual code changes without the accidental Git LFS file expansions.
cmnbroad
left a comment
cmnbroad
left a comment
There was a problem hiding this comment.
@MarcoLotz Sorry for the delay in getting back to you on this. Unfortunately I don't think there is very much here we can take. Its not a goal to make Windows a supported platform, and maintaining CI tests on Windows is not a path we want to go down. Additionally, some of these changes are likely to introduce new issues on the existing platforms (I commented on some of them inline, but I think there are others).
| # Ensure reference files use LF line endings to maintain consistent MD5 checksums across platforms | ||
| *.fasta text eol=lf | ||
| *.fa text eol=lf | ||
| *.fai text eol=lf |
There was a problem hiding this comment.
The problem with doing this is that it gives the illusion of solving the line ending problem, but it's only a partial fix and can result in subtle problems downstream. If someone creates a .fa./.fai pair on Windows, git will change the line endings in the .fa, which will then invalidate the offsets in the .fai and cause failures on non-Windows devices.
| if (tmpDir == null) { | ||
| tmpDir = new GATKPath(System.getProperty("java.io.tmpdir")); | ||
| // Use File.getAbsolutePath() which handles Windows paths correctly | ||
| tmpDir = new GATKPath(new File(System.getProperty("java.io.tmpdir")).getAbsolutePath()); |
There was a problem hiding this comment.
The File constructor is not equipped to handle URIs that contains protocol components; failures that result from interconversion to File is one of the main reasons we introduced the IOPath/GATKPath classes. Although there are still places in the code where this pattern is used, it is likely to cause problems here.
| // On Windows, strings like "1:1-100" (interval notation) will throw InvalidPathException | ||
| // because ':' is not allowed except after drive letters. Return a placeholder path | ||
| // that will fail existence checks, allowing the caller to treat it as an interval string. | ||
| return Paths.get(uriString.replaceAll(":", "_")); |
There was a problem hiding this comment.
This seems pretty sketchy. In addition to the fact that its misleading in the case where the user actually specified a path that is invalid, I'm also unclear on how its helpful to return something that is neither a valid path nor a valid interval string.
|
We don't plan to support the Windows platform. |
2 participants
The reasoning, motivation, and status quo are provided in #9293.
As mentioned in the issue, Windows is currently not supported for GATK.
This PR creates a CI/CD flow for Windows to enable development support and flag regressions.
Addresses all Windows failing tests.
This PR focuses on making small changes to the Code Under Test to be platform-independent, as well as small changes on tests that are not possible to be implemented under windows platform. In general, the changes are:
Notes:
Closes #9293.