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

use cygpath if wsl is not available #852

Closed
wants to merge 1 commit into from

Conversation

Emilgardis
Copy link
Member

@Emilgardis Emilgardis commented Jun 24, 2022

Implements getting unix paths with cygpath if wsl/wslpath is not installed.

Error:
   0: could not get linux compatible path for `"G:\\workspace\\small dev space\\test-workspace\\workspace"`

Error:
   0: could not find cygpath.exe

Error:
   0: could not find wslpath.exe

Warning: usage of `env.volumes` requires WSL or cygpath on Windows
Suggestion: is WSL or cygpath installed on the host?

@Emilgardis Emilgardis added the no changelog A valid PR without changelog (no-changelog) label Jun 24, 2022
@Emilgardis Emilgardis requested a review from a team as a code owner June 24, 2022 18:11
Comment on lines +350 to +359
.with_error(|| {
<eyre::Report as std::convert::AsRef<
dyn std::error::Error + Send + Sync + 'static,
>>::as_ref(Box::leak(Box::new(cyg_err)))
})
.with_error(|| {
<eyre::Report as std::convert::AsRef<
dyn std::error::Error + Send + Sync + 'static,
>>::as_ref(Box::leak(Box::new(wsl_err)))
})
Copy link
Member Author

Choose a reason for hiding this comment

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

some nice leaks, unfortunately couldn't find a better way to do this

Comment on lines 444 to +445
// podman weirdly expects a WSL path here, and fails otherwise
path = wslpath(&path, verbose)?;
path = winpath(&path, verbose)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

does this work with podman?

@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jun 24, 2022

Isn't WSL2 required though? Both Docker and Podman require it on Windows. So we might have an issue with it not being present, and maybe we need a better error?

@Emilgardis
Copy link
Member Author

wsl2 is not required, only wsl. doing this specifically because of the error in #851 : https://github.com/cross-rs/cross/runs/7045847730?check_suite_focus=true

@Alexhuszagh
Copy link
Contributor

wsl2 is not required, only wsl. doing this specifically because of the error in #851 : https://github.com/cross-rs/cross/runs/7045847730?check_suite_focus=true

I believe WSL2 is now required, or Hyper-V (I forgot that LinuxKit was a thing). I know WSL used to be used back in 2017ish? 2016ish? back when I used it then before Docker Desktop was released, but I doubt we should rely on anyone used that anymore. We probably need a different solution than cygpath if we do, since we would want it to be compatible with Hyper-V?

@Emilgardis
Copy link
Member Author

Putting the discussion from matrix here:

We probably need a different solution than cygpath if we do, since we would want it to be compatible with Hyper-V?

We don't need the paths to be compatible, only that they are expressed in a sane way that unix understands.

This is because we mount the volume paths with -v C:\path\to\thing:/mnt/c/path/to/thing

We could instead of using cygpath fallback to something like relative_path

@Emilgardis Emilgardis marked this pull request as draft June 24, 2022 19:29
@Alexhuszagh
Copy link
Contributor

Alexhuszagh commented Jun 24, 2022

I think this won't be too complex to implement in cross itself. We shouldn't have to worry about UNC paths or I believe verbatim paths as long as we canonicalize them first in most cases, except when necessary.

@Emilgardis
Copy link
Member Author

docker -v doesn't take unc paths, don't think there are cases where we use unc paths

@Alexhuszagh
Copy link
Contributor

docker -v doesn't take unc paths, don't think there are cases where we use unc paths

If that's the case, then handling drives is pretty easy.

Alexhuszagh added a commit to Alexhuszagh/cross that referenced this pull request Jun 24, 2022
Removes the dependency on WSL2, and properly handles DOS-style paths,
UNC paths, including those on localhost, even if Docker itself does not
support them.

Closes cross-rs#854.
Related to cross-rs#665.
Supersedes cross-rs#852.
bors bot added a commit that referenced this pull request Jun 24, 2022
856: Remove WSLpath and implement WSL-style path. r=Emilgardis a=Alexhuszagh

Removes the dependency on WSL2, and properly handles DOS-style paths, UNC paths, including those on localhost, even if Docker itself does not support them.

Closes #854.
Related to #665.
Supersedes #852.

Co-authored-by: Alex Huszagh <[email protected]>
@Emilgardis Emilgardis closed this Jun 24, 2022
@Alexhuszagh Alexhuszagh added the A-windows-host Area: windows (not WSL2) hosts label Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-windows-host Area: windows (not WSL2) hosts no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants