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

fix(wash): windows path to target #1486

Merged
merged 4 commits into from
Feb 13, 2024
Merged

fix(wash): windows path to target #1486

merged 4 commits into from
Feb 13, 2024

Conversation

ricochet
Copy link
Contributor

@ricochet ricochet commented Feb 9, 2024

Verbatim paths on Windows are not well supported,
e.g. "\\?\C:\Users..." while technically valid, causes some fs api's like exists to fail errantly.

The fix is to use a third party lib normpath to normalize the path to the wasm
binary.

Related issue: rust-lang/cargo#9770

Fixes #1481

Signed-off-by: Bailey Hayes behayes2@gmail.com

@ricochet ricochet requested review from a team as code owners February 9, 2024 04:05
t3hmrman

This comment was marked as duplicate.

Copy link
Contributor

@vados-cosmonic vados-cosmonic left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Would love a test for this so we can catch it in the future when we change/to make sure it keeps not being broken, but that can be a follow up thing!

Cargo.toml Outdated Show resolved Hide resolved
@vados-cosmonic vados-cosmonic removed the request for review from t3hmrman February 9, 2024 13:01
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

Just a note we'll need to make sure this makes it into the upcoming wash-lib release so we can release the fix in wash-cli

@connorsmith256
Copy link
Contributor

Hmm, it looks like a lot of the wash build integration tests are failing

Verbatim paths on Windows are not well supported,
e.g. "\\\\?\\C:\\Users..." while technically valid, causes some fs api's like `exists` to fail errantly.

The fix is to use a third party lib normpath to normalize the path to the wasm
binary.

Related issue: rust-lang/cargo#9770

Signed-off-by: Bailey Hayes <behayes2@gmail.com>
Co-authored-by: Victor Adossi <123968127+vados-cosmonic@users.noreply.github.com>
@ricochet
Copy link
Contributor Author

Integration tests caught a bug with what I added. Normalize uses the fs to run the normalization and the file must exist. Tweaked this a bit and re-tested on windows and macos.

Signed-off-by: Connor Smith <connor.smith.256@gmail.com>
@connorsmith256 connorsmith256 mentioned this pull request Feb 12, 2024
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
@brooksmtownsend brooksmtownsend enabled auto-merge (rebase) February 13, 2024 16:31
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
@brooksmtownsend brooksmtownsend merged commit d013140 into main Feb 13, 2024
33 checks passed
@brooksmtownsend brooksmtownsend deleted the w-paths branch February 13, 2024 17:04
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.

[BUG] wash build on windows appears to be broken
5 participants