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.

Sign up for GitHub

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

Jump to bottom

windows: opam root redirection when path contains spaces #5457

Merged
merged 6 commits into from
Jun 10, 2024
Merged

windows: opam root redirection when path contains spaces #5457

merged 6 commits into from
Jun 10, 2024

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Feb 28, 2023 •
edited by kit-ty-kate
Loading

On Windows, opam may install a local cygwin, but cygwin doesn't support having a space in its path. We need then to redirect all opam root into a path with no space. Redirection is done at first init, and afterwards it is followed for each opam call.
So opam root directory can contain either:

  • an unique file redirected-opamroot that contains the new path

  • or the already known opam root.

  • update changes

  • update message with relevant root dir

  • set default path, or give user the choice

  • add an error if --root/OPAMROOT path contains space

  • add readme.txt in C:\opamroot

related to #5220

@rjbou rjbou added AREA: PORTABILITY PR: WIP Not for merge at this stage labels Feb 28, 2023
@rjbou rjbou added this to the 2.2.0~alpha milestone Feb 28, 2023
@rjbou rjbou requested a review from dra27 February 28, 2023 13:34
@rjbou rjbou removed this from the 2.2.0~alpha milestone Jun 7, 2023
@kit-ty-kate
Copy link
Member

note from dev meeting: for the directory we could create a new directory in C:\, it should be possible to create one without admin priviledges

@rjbou rjbou self-assigned this May 27, 2024
@rjbou rjbou removed the PR: WIP Not for merge at this stage label Jun 3, 2024
@kit-ty-kate kit-ty-kate self-requested a review June 4, 2024 12:26
rjbou
rjbou commented Jun 4, 2024
View reviewed changes
src/client/opamClient.ml Outdated Show resolved Hide resolved
kit-ty-kate
kit-ty-kate reviewed Jun 4, 2024
View reviewed changes
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

General question: why do we redirect the entire opam root instead of just installing Cygwin in C:\opam-cygwin or similar? What are the advantage/inconvenient of both solutions?

src/client/opamClient.ml Outdated Show resolved Hide resolved
src/client/opamClient.ml Outdated Show resolved Hide resolved
src/client/opamClientConfig.ml Outdated Show resolved Hide resolved
tests/reftests/env.test
-> installed nv.1
Done.
# Run eval $(opam env '--root=${BASEDIR}/root 2' '--switch=${BASEDIR}/switch w spaces') to update the current shell environment
### opam env --root "$RT" --switch "./$SW" | grep "NV_VARS" | ';' -> ':'
Copy link
Member

Choose a reason for hiding this comment

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

why is this test not needed anymore? What was it testing to being with?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is moved to env.unix & init.win32 as the behaviour differs

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing that command in either of those files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah,the command itself, true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that originally it was here to show that the switch was well created?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

The new one doesn't seem to do the same thing grep NV_VARS vs grep PREFIX. The original line was added by #5476 which seems to have another purpose. cc @dra27

Copy link
Member

Choose a reason for hiding this comment

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

This is safe - the main thing is that the primary tests in #5935 (further up the file) are still there. The noise in these other parts suggests that the tests should possibly be split up a bit further (but's definitely a post 2.2.0 thing!)

rjbou
rjbou commented Jun 5, 2024
View reviewed changes
src/client/opamClientConfig.ml Outdated Show resolved Hide resolved
@rjbou rjbou requested a review from kit-ty-kate June 5, 2024 17:55
@kit-ty-kate
Copy link
Member

General question: why do we redirect the entire opam root instead of just installing Cygwin in C:\opam-cygwin or similar? What are the advantage/inconvenient of both solutions?

my question wasn't answered. Looks fine otherwise on a glance, i'll test it tomorrow

@rjbou
Copy link
Collaborator Author

rjbou commented Jun 6, 2024 •
edited
Loading

I know, i hadn't time :)
The main point, if I remember well, is to keep everything under the opam root. Besides, users are used to delete opam root to restart from scratch. If internal cygwin is a separated directory, it can lead to partial install left in that case.
I agree that on purely technical part, it'll be easier (and less changes) if we store only the Cygwin install under C:.
@dra27 Do you remember another point we discussed at the time ?

@kit-ty-kate kit-ty-kate force-pushed the opamroot-redirect branch 5 times, most recently from 94ef0e4 to 1883aac Compare June 7, 2024 18:14
@kit-ty-kate kit-ty-kate mentioned this pull request Jun 7, 2024
Merged
3 tasks
dra27
dra27 reviewed Jun 9, 2024
View reviewed changes
tests/reftests/init.win32.test Outdated
-> test test
### echo $OPAMROOT
roots/with path
### rm -rf 'C:\opamroot'
Copy link
Member

Choose a reason for hiding this comment

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

This definitely can't stay - it blows away the user's opam root!

Copy link
Member

Choose a reason for hiding this comment

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

but then the tests are not reproducible on Windows at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's ok for CI, on tester computer, we can add a note about it in the test and not remove something outside the test directories

@dra27 dra27 added this to the 2.2.0~beta3 milestone Jun 9, 2024
@kit-ty-kate kit-ty-kate mentioned this pull request Jun 10, 2024
Draft
@kit-ty-kate
Copy link
Member

kit-ty-kate commented Jun 10, 2024 •
edited
Loading

The tests have been split off to #6006

dra27 and others added 2 commits June 10, 2024 15:29
@dra27 @rjbou
system: add mk_unique_dir
01ed890
@rjbou @dra27
Redirect opam on Windows if path contains a space.
c43f7b4 
It is needed for Cygwin installation, that doesn't handle paths with
space.
At init, detection and redirection are done, afterwards opam always load
redirected opam root.
Original root directory is stored in
`OpamStateConfig.!r.original_root_dir`.

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
@rjbou
Copy link
Collaborator Author

rjbou commented Jun 10, 2024

🎉

@rjbou rjbou merged commit a879424 into ocaml:master Jun 10, 2024
29 checks passed
@rjbou
Copy link
Collaborator Author

rjbou commented Jun 10, 2024

Thanks all!

@dra27 dra27 mentioned this pull request Jun 13, 2024
Closed
This was referenced Jun 21, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dra27 dra27 dra27 left review comments

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

Assignees

@rjbou rjbou

Labels
AREA: PORTABILITY
Projects
None yet
Milestone
2.2.0~beta3
Development

Successfully merging this pull request may close these issues.
3 participants
@rjbou @kit-ty-kate @dra27