Skip to content
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

Support MSYS2 #4813

Merged
merged 2 commits into from
Sep 1, 2021
Merged

Support MSYS2 #4813

merged 2 commits into from
Sep 1, 2021

Conversation

jonahbeckford
Copy link
Contributor

This is a follow-up to the announcement made at https://discuss.ocaml.org/t/ann-windows-friendly-ocaml-4-12-distribution-diskuv-ocaml-0-1-0/8358

Key Changes:

  • Use USERPROFILE environment variable on Windows if HOME is not defined
  • For the most part treat MSYS2 and Cygwin as equivalent. MSYS2 executables can be detected as msys-2.0.dll.
  • Symlinks are not portably supported on Windows. Solutions like requiring the developer mode of Windows or Administrator access for NTFS symlinks will always limit the audience on Windows; I don't want to limit the audience. For Opam packages with symlinks the MSYS2 environment will simply default to copying rather than symlinking, but copying has edge cases:
    1. When rsync-ing the symlink referent must exist before the symlink (ie. copy) is made. In this PR I've dealt with that by doing a two-pass rsync on MSYS2; the first pass syncs all the files except the symlink, while the second pass does the symlinks.
    2. tar.exe may fail on these same source packages; I can't use the two-pass trick. There isn't a problem if the .tar is packaged in topological order (referents come before the symlinks) but that presumes a lot. For now I'm just creating branches of the problematic source packages (ex. https://github.com/diskuv/ocp-indent/commits/windows-support), but I suspect a real solution may require a patch to tar.exe on Windows to add an option to skip symlinks.
  • There is a PowerShell replacement for eval $(opam env) that I've included in this PR

Extra guidance needed:


Please update master_changes.md file with your changes.

I'm not sure which section to put these changes.

@kit-ty-kate kit-ty-kate requested a review from dra27 August 29, 2021 22:53
@kit-ty-kate kit-ty-kate added this to PR in progress in Opam 2.2.0 via automation Aug 29, 2021
@kit-ty-kate kit-ty-kate added this to the 2.2.0~alpha milestone Aug 29, 2021
@dra27
Copy link
Member

dra27 commented Aug 30, 2021

Thanks very much for all the work you're doing both on this and OCaml on Windows in general! Would it be OK to split this up:

  • I'm instantly happy with 939acbc ("Support MSYS2") and 2b60006 ("Add .vscode to .gitignore")
  • For a24d269 ("Support USERPROFILE as HOME fallback for Windows"), there's prior art in dra27@ed44880. The choice of directory (user documents) in that one is bad - in fact, my current intention is to address this "properly" as part of Support ~/.config/opam (per XDG base dir spec) #3766 in opam 2.2. My issue with falling back to %USERPROFILE% is that it's not a directory which we should be writing in (my enterprise setup, for example, uses both redirected folders and roaming profiles).
  • For 92397ac ("Allow MSYS2 to prefer copying over symlinking") I think the code duplication can be reduced if is_cygwin_variant and is_msys2_variant are merged into a single function get_windows_executable_variant : string -> [ `Native | `Cygwin of [`Cygwin | `Msys2] | `Tainted of [`Cygwin | `Msys2] ]. The old is_cygwin_variant and the new is_msys2_variant can then be defined in terms of that function. While it's being added, it would be good to thread the tainting support through to OpamSystem.install (the point of `CygLinked is to warn for the situation where you've accidentally linked with a Cygwin or MSYS2 compiled DLL which usually implies bindings are wrong somewhere). I was initially confused by the title of the commit, as we must definitely prefer symlinking over copying (if it's available and works), vs requiring symlinking (I completely agree with you that requiring developer mode is an unacceptable barrier), but I think that the actual rsync change works correctly if native symlinking is enabled. The longer-term plan for this (I say longer, it's possible this'll happen in 2.2 or 2.3) is to use ocaml-tar and implement the cp command directly. Cygwin/MSYS2' cp and tar commands are somewhat belligerently staying portable where symlinks are concerned - Windows symlinks do not require the file to exist, what they do require is for the type of the target to be known, which cp does (trivially) and tar can (slightly less-trivially). The only thing which is very hard to copy reliably is a dangling/broken symlink. However, I think your rsync solution is a very good stop-gap!
  • For 0f0bd98 ("Show eval expression for PowerShell on native Windows"), opam 2.2 should finally see the activation of parent_putenv (and the opam-putenv auxiliary)... on Windows, there's no need to output shell instructions - on Windows, opam env can actually update the environment variables of the shell (see dra27@2934dc4#diff-e21e37a0339dbaafa95fd0b6d57221f656f18d0771c06f6f837bb98c3a71b91aR193). However, that doesn't necessarily preclude merging this first!

@jonahbeckford
Copy link
Contributor Author

The PR has been split.

This current PR is now just 939acbc ("Support MSYS2") and 2b60006 ("Add .vscode to .gitignore")

@jonahbeckford jonahbeckford changed the title Support Microsoft toolchain and MSYS2 Support MSYS2 Aug 30, 2021
@rjbou
Copy link
Collaborator

rjbou commented Aug 30, 2021

Thanks for the PR and the update. On the changelog, you cant just add it in Internal section. I can't push on your branch:

diff --git a/master_changes.md b/master_changes.md
index 092da2d29..bd26a9037 100644
--- a/master_changes.md
+++ b/master_changes.md
@@ -111,6 +111,7 @@ users)
 ## Internal
   * Add license and lowerbounds to opam files [#4714 @kit-ty-kate]
   * Bump version to 2.2.0~alpha~dev [#4725 @dra27]
+  * Support MSYS2:  treat MSYS2 and Cygwin as equivalent [#4813 @diskuv]

@jonahbeckford
Copy link
Contributor Author

jonahbeckford commented Aug 30, 2021

Added the change and added access for you and @dra27 (can add others if needed). Thanks!

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Good to go with CI, thanks!

Opam 2.2.0 automation moved this from PR in progress to PR finalised Aug 31, 2021
@dra27
Copy link
Member

dra27 commented Aug 31, 2021

Added the change and added access for you and @dra27 (can add others if needed). Thanks!

Thanks, although this gives more access to us than you should really want! I don't know if it's officially documented by GitHub, but it seems to be an issue with making pull requests from one organisation's repositories to another, so you might find it easier when upstreaming to push the branches to a fork on @jonahbeckford instead.

@rjbou rjbou merged commit 727c523 into ocaml:master Sep 1, 2021
Opam 2.2.0 automation moved this from PR finalised to Done Sep 1, 2021
@jonahbeckford jonahbeckford deleted the feature-msvc-msys2 branch May 3, 2022 18:05
@dra27 dra27 mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants