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

Create opam root in %LOCALAPPDATA%\opam on Windows #5212

Merged
merged 3 commits into from
Sep 1, 2022

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Aug 2, 2022

If .opam is found in either %HOME% or %USERPROFILE% then use that, but roots are otherwise created in %LOCALAPPDATA%\opam.

This strategy is forwards-compatible with any switch to XDG (which would definitely introduce the situation where existing roots would be read from locations other than where a new root would be init'd). The location for Windows was discussed in #3766.

master_changes.md Show resolved Hide resolved
@jonahbeckford
Copy link
Contributor

This is great; ordinary opam.exe will be able to read OPAMROOT from Diskuv's opam.exe.

@dra27
Copy link
Member Author

dra27 commented Aug 2, 2022

The use of %HOME% also makes it compatible with opam-repository-mingw

@jonahbeckford
Copy link
Contributor

LGTM

@dra27
Copy link
Member Author

dra27 commented Aug 3, 2022

Looks like a tweak is needed to the runners. opam init is also incorrectly displaying ~\.opam as the install location.

@dra27 dra27 mentioned this pull request Aug 3, 2022
16 tasks
@dra27 dra27 added this to the 2.2.0~alpha milestone Aug 4, 2022
If .opam is found in either %HOME% or %USERPROFILE% then use that, but
roots are otherwise created in %LOCALAPPDATA%\opam.
The directory before was selected "by fluke" - the CI runners don't want
to use C:\ by default.
The hard-coded default of ~/.opam is not correct on Windows and also not
technically correct if the user specified --root or set the OPAMROOT
environment variable. This commit fixes both - ~/.opam is displayed when
correct, and a more detailed message is displayed (mentioning the
default value) if it has been overridden.
@dra27
Copy link
Member Author

dra27 commented Aug 8, 2022

Thanks for the review, @AltGr! I've hopefully fixed the CI - beforehand, OpamStd.Sys.home was falling to the default of .opam in the CWD which was a dreadful default for Windows, but actually correct for the runners (that's fixed by setting OPAMROOT)

There's an extra commit to be checked, though. I've altered the code so that you now get the correct display on Windows:

  In normal operation, opam only alters files within ~\AppData\Local\opam.

but while there it was worth fixing #4992 as well. If the root is not the default, then you get this slightly different message:

  In normal operation, opam only alters files within your opam root
    (~\AppData\Local\opam by default; currently C:\Devel\opam-2\root).

We could possibly be more explicit about why the root has been overridden (there are three possibilities: --root, OPAMROOT or a legacy location - i.e. somewhere a root is read from but not written like %HOME%\.opam on Windows) but I expect that it's fine as it is.

@kit-ty-kate
Copy link
Member

Thanks a lot!

@kit-ty-kate kit-ty-kate merged commit 2a29c40 into ocaml:master Sep 1, 2022
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required setup message contains incorrect path to opam root when using OPAMROOT
5 participants