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

UNC Paths and compilation on Windows #457

Closed
wants to merge 5 commits into from
Closed

Conversation

rutexd
Copy link

@rutexd rutexd commented Nov 3, 2022

Fixes:

  1. Gateway service compile error: Since Shuttle is Docker and Linux first oriented project, Shuttle gateway service uses Docker linux-first functions which defined through conditional compiling by cfg. This is the issue under windows, since you have to cut out project member from compilation by hand.
    Replacing Docker::connect_with_unix with Docker::connect_with_local does the same thing as before for unix but allows to be compiled under windows, for cases where if you want to complie Shuttle-CLI or work with other non-server stuff.

  2. Fixes (partially) Windows paths for cargo-shuttle deployer. Since Rust tries to support new Windows UNC paths, which are new and compatibility breaking, this support isnt perfect and rust has some bugs with it. As the example - cargo package may break the package on git initialized project in some unclear conditions and generate instead of working package - package filled with zeros only because of UNC path.

@@ -183,7 +183,26 @@ pub struct InitArgs {

// Helper function to parse and return the absolute path
fn parse_path(path: &OsStr) -> Result<PathBuf, io::Error> {
canonicalize(path).map_err(|e| {
#[cfg(target_os = "windows")]
Copy link
Contributor

Choose a reason for hiding this comment

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

@rutexd I really want to merge this. But will prefer a solution where we don't use cfg. Ie the same non-canonicalize code should work on both Windows and Unix. If you can push such an update, then we can merge this

@oddgrd
Copy link
Contributor

oddgrd commented Mar 22, 2023

Thanks for the effort on this @rutexd! 🙏 We fixed this in #721, so I'll close this.

@oddgrd oddgrd closed this Mar 22, 2023
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.

3 participants