Skip to content

fix: Correctly set os-distribution on Windows#13536

Merged
Alizter merged 1 commit intoocaml:mainfrom
punchagan:win32-os-distribution
Feb 5, 2026
Merged

fix: Correctly set os-distribution on Windows#13536
Alizter merged 1 commit intoocaml:mainfrom
punchagan:win32-os-distribution

Conversation

@punchagan
Copy link
Copy Markdown
Collaborator

@punchagan punchagan commented Feb 3, 2026

Currently, we return a stub value that matches the OS name on Windows. But, the os-distribution needs to be cygwin or msys2 when building in Cygwin or MSYS2, respectively, to detect dependencies correctly for various packages.

We assume that the uname executable is on PATH when running under Cygwin or MSYS2, and use that to correctly detect the os-distribution.

Closes #13310

@punchagan
Copy link
Copy Markdown
Collaborator Author

punchagan commented Feb 4, 2026 •
edited
Loading

opam uses a utility function get_windows_executable_variant to detect if the cygpath.exe binary is Native, Cygwin or MSYS2 binary, based on whether it links to the the cygwin1.dll or the msys-2.0.dll or depends on a library that links to one of them. The utility function seems to have been originally written to detect if exectuables like tar or rsync being used by opam were Cygwin tools since they don't accept Windows like paths.

I'm not sure we need to use the same method. I think it's just assuming uname would be on PATH when building under Cygwin or MSYS2, and using that to determine the os-distribution seems reasonable to me.

@punchagan punchagan marked this pull request as ready for review February 4, 2026 10:55
@punchagan punchagan force-pushed the win32-os-distribution branch from b08e46f to 7c1c646 Compare February 4, 2026 10:55
Leonidas-from-XIV
Leonidas-from-XIV approved these changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have cygwin nor msys at hand, but it seems ok to call uname instead of inspecting a binary.

@punchagan
fix: Correctly set os-distribution on Windows
fbac750 
Currently, we return a stub value that matches the OS name on Windows.
But, the `os-distribution` needs to be `cygwin` or `msys2` when building
in Cygwin or MSYS2, respectively, to detect dependencies correctly for
various packages.

We assume that the `uname` executable is on `PATH` when running under
Cygwin or MSYS2, and use that to correctly detect the `os-distribution`.

Closes ocaml#13310

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
@punchagan punchagan force-pushed the win32-os-distribution branch from 7c1c646 to fbac750 Compare February 4, 2026 15:47
Alizter
Alizter approved these changes Feb 5, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked on Windows and it is doing the right thing.

@Alizter Alizter merged commit bb6473a into ocaml:main Feb 5, 2026
28 checks passed
@punchagan punchagan deleted the win32-os-distribution branch February 5, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Leonidas-from-XIV Leonidas-from-XIV Leonidas-from-XIV approved these changes

@Alizter Alizter Alizter approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve os_distribution detection in dune pkg on Windows

3 participants

@punchagan @Leonidas-from-XIV @Alizter