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

make env.volumes only change package path when used #665

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

Emilgardis
Copy link
Member

@Emilgardis Emilgardis commented Mar 17, 2022

This fixes windows usage.

It also tries to fix windows builds when using env.volumes by making windows compatible
paths for env.volumes

not using env.volumes will now also mount the project in /project again

src/docker.rs Outdated Show resolved Hide resolved
src/docker.rs Outdated Show resolved Hide resolved
src/docker.rs Outdated Show resolved Hide resolved
@Emilgardis
Copy link
Member Author

I have not tested this with env.volumes yet

@Emilgardis
Copy link
Member Author

bors try --target x86

bors bot added a commit that referenced this pull request Mar 17, 2022
@bors
Copy link
Contributor

bors bot commented Mar 17, 2022

try

Build succeeded:

@Emilgardis Emilgardis force-pushed the optional-envvolumes branch 4 times, most recently from 1679481 to b29313c Compare March 18, 2022 23:22
@Emilgardis Emilgardis marked this pull request as ready for review March 18, 2022 23:22
@Emilgardis
Copy link
Member Author

Emilgardis commented Mar 18, 2022

This should be feature complete now 🎉

Example docker invocation on windows

with env.volumes

docker.EXE run
--userns "host"
-v "G:\\workspace\\cross_testing\\hello.txt:/mnt/g/workspace/cross_testing/hello.txt"
-e "HELLO=/mnt/g/workspace/cross_testing/hello.txt"
-e "PKG_CONFIG_ALLOW_CROSS=1"
--rm -e "XARGO_HOME=/xargo"
-e "CARGO_HOME=/cargo"
-e "CARGO_TARGET_DIR=/target"
-e "USER=Emil"
-e "CROSS_RUNNER="
-v "C:\\Users\\Emil\\.xargo:/xargo:Z"
-v "C:\\Users\\Emil\\.cargo:/cargo:Z"
-v "/cargo/bin"
-v "G:\\workspace\\cross_testing\\foo:/mnt/g/workspace/cross_testing/foo:Z"
-v "C:\\Users\\Emil\\.rustup\\toolchains\\nightly-x86_64-unknown-linux-gnu:/rust:Z,ro"
-v "G:\\workspace\\cross_testing\\foo\\target:/target:Z"
-w "/mnt/g/workspace/cross_testing/foo"
-i -t "ghcr.io/cross-rs/aarch64-unknown-linux-gnu:edge"
sh
-c "PATH=$PATH:/rust/bin cargo run -vv --target aarch64-unknown-linux-gnu"

without

docker.EXE run 
--userns host 
-e "PKG_CONFIG_ALLOW_CROSS=1" 
--rm 
-e "XARGO_HOME=/xargo" 
-e "CARGO_HOME=/cargo" 
-e "CARGO_TARGET_DIR=/target" 
-e "USER=Emil" 
-e "CROSS_RUNNER=" 
-v "C:\\Users\\Emil\\.xargo:/xargo:Z" 
-v "C:\\Users\\Emil\\.cargo:/cargo:Z" 
-v "/cargo/bin" 
-v "G:\\workspace\\cross_testing\\foo:/project:Z" 
-v "C:\\Users\\Emil\\.rustup\\toolchains\\nightly-x86_64-unknown-linux-gnu:/rust:Z,ro" 
-v "G:\\workspace\\small dev space\\cross_testing\\foo\\target:/target:Z" 
-w "/project" 
-i  -t "ghcr.io/cross-rs/aarch64-unknown-linux-gnu:edge"
sh 
-c "PATH=$PATH:/rust/bin cargo run -vv --target aarch64-unknown-linux-gnu"

Comment on lines +274 to +293
fn wslpath(path: &Path, verbose: bool) -> Result<PathBuf> {
let wslpath = which::which("wsl.exe")
.map_err(|_| eyre::eyre!("could not find wsl.exe"))
.warning("usage of `env.volumes` requires WSL on Windows")
.suggestion("is WSL installed on the host?")?;

Command::new(wslpath)
.arg("-e")
.arg("wslpath")
.arg("-a")
.arg(path)
.run_and_get_stdout(verbose)
.wrap_err_with(|| {
format!(
"could not get linux compatible path for `{}`",
path.display()
)
})
.map(|s| s.trim().into())
}
Copy link
Member Author

@Emilgardis Emilgardis Mar 18, 2022

Choose a reason for hiding this comment

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

it would be possible to fallback to a manual implementation of "normalizing". the logic should be quite simple but I don't feel like it's necessary to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take that back, I tried to do it correctly but handling this is quite annoying :)

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe

fn wslpath2(path: impl AsRef<Path>) -> Result<String> {
    fn get_path_prefix_drive(path: &Path) -> Option<&str> {
        match path.components().next().unwrap() {
            std::path::Component::Prefix(prefix_component) => match prefix_component.kind() {
                std::path::Prefix::VerbatimDisk(_) => Some(
                    prefix_component
                        .as_os_str()
                        .to_str()
                        .expect("windows drive letters should be ascii A-Z")
                        .strip_prefix(r"\\?\")?
                        .strip_suffix(":")?,
                ),
                std::path::Prefix::Disk(_) => Some(
                    prefix_component
                        .as_os_str()
                        .to_str()
                        .expect("windows drive letters should be ascii A-Z")
                        .strip_suffix(":")?,
                ),
                _ => None,
            },
            _ => None,
        }
    }
    let path = path.as_ref();
    let components = path.components().skip(2);
    let mut path_c = String::from("/mnt/");

    path_c
        .push_str(&get_path_prefix_drive(path).ok_or_else(|| eyre::eyre!("no drive letter found"))?.to_lowercase());
    let mut sep = true;
    for comp in components {
        if sep {
            path_c.push('/');
        } else {
            sep = true;
        }
        match comp {
            std::path::Component::Normal(p) => path_c.push_str(
                p.to_str()
                    .ok_or_else(|| eyre::eyre!("path needs to be utf8"))?,
            ),
            std::path::Component::ParentDir => path_c.push_str(".."),
            std::path::Component::CurDir => path_c.push('.'),
            _ => sep = false,
        }
    }
    Ok(path_c)
}

@Emilgardis
Copy link
Member Author

has anyone been able to test this and give this a r+ @cross-rs/maintainers <3

this also tries to fix windows builds by making windows compatible
paths for env.volumes
@Emilgardis Emilgardis force-pushed the optional-envvolumes branch from f79ce98 to daa9ced Compare March 29, 2022 16:01
@Emilgardis
Copy link
Member Author

ping @cross-rs/maintainers

It would be good to have this included for 0.3.0

@reitermarkus
Copy link
Member

I'm not using Windows so I cannot test this, but it looks good so far.

not using env.volumes will now also mount the project in /project again

Does this have any advantage or is this needed to fix Windows support?

@Emilgardis
Copy link
Member Author

not using env.volumes will now also mount the project in /project again

Does this have any advantage or is this needed to fix Windows support?

No advantage, except for not leaking paths in the general case, like what was done before.

bors r=reitermarkus

@bors
Copy link
Contributor

bors bot commented Mar 31, 2022

Build succeeded:

@bors bors bot merged commit 39c4d5b into cross-rs:main Mar 31, 2022
@Emilgardis Emilgardis deleted the optional-envvolumes branch April 7, 2022 06:42
@Emilgardis Emilgardis added this to the v0.2.2 milestone Jun 15, 2022
@Emilgardis Emilgardis mentioned this pull request Jun 24, 2022
11 tasks
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.
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.
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]>
@Alexhuszagh Alexhuszagh added enhancement A-windows-host Area: windows (not WSL2) hosts labels 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 enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants