Skip to content

Patch fstab on Cygwin temporarily #975

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

Merged
merged 1 commit into from
Apr 22, 2025
Merged

Patch fstab on Cygwin temporarily #975

merged 1 commit into from
Apr 22, 2025

Conversation

smorimoto
Copy link
Member

Closes #972

Signed-off-by: Sora Morimoto <sora@morimoto.io>
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR temporarily patches the Cygwin fstab configuration to address issue #972. Key changes include:

  • Adding a new function fixFstab() in windows.ts to modify the fstab file.
  • Updating installer.ts to import and invoke fixFstab() after setting up the environment.
  • Introducing additional file system and path modules to support the patch.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/setup-ocaml/src/windows.ts Adds fixFstab() to patch the fstab file
packages/setup-ocaml/src/installer.ts Imports and calls fixFstab() in the installer flow
Comments suppressed due to low confidence (1)

packages/setup-ocaml/src/windows.ts:93

  • Ensure that the Node.js version in use supports lookbehind assertions in regular expressions, as used in this patch substitution. Without support, the replacement may fail at runtime.
const patchedContents = contents.replace(

@smorimoto smorimoto merged commit 95e8edb into master Apr 22, 2025
18 checks passed
@smorimoto smorimoto deleted the fix-fstab branch April 22, 2025 04:48
@dra27
Copy link
Member

dra27 commented Apr 24, 2025

Assuming this doesn't lead to any problems, it should be a permanent change - it brings setup-ocaml into line with how opam manages Cygwin as well (it's also philosophically correct: the opam root is a native Windows directory so it shouldn't be being manipulated with POSIX permissions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants