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 issue with windows local manifest paths being absolute #2842

Merged
merged 2 commits into from
Mar 30, 2024

Conversation

Acepie
Copy link
Contributor

@Acepie Acepie commented Mar 26, 2024

Closes #2837

The issue was that on Windows canonicalise adds a special root prefix https://stackoverflow.com/questions/41233684/why-does-my-canonicalized-path-get-prefixed-with which caused the diff to not properly work when creating the relative path. Since keeping the target canonical might be important for resolutions in some cases this pr updates the source to be canonicalized when doing the relative path.

I tested this against https://github.com/Acepie/p5js_gleam locally on my windows computer and it seems to work as expected now.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Could you add a test for this please? Thank you 🙏

@Acepie
Copy link
Contributor Author

Acepie commented Mar 26, 2024

So the difficulty for adding a test for this case is that canonicalize actually checks the file exists in the filesystem, which is valid/always true in real-world usage but makes it pretty hard to make a nonbrittle test that hits this case

@Acepie
Copy link
Contributor Author

Acepie commented Mar 26, 2024

Or at the very least, i don't know how we'd write a test that passes consistently on both local and CI that actually tests the intended behavior (i.e without writing some weird mock that just string appends the root without an FS check)

@Acepie
Copy link
Contributor Author

Acepie commented Mar 29, 2024

Ok based on how difficult it was to test the canonicalising for source and from what I read in rust-lang/rust#42869 it seems like we might be better off just stripping from target. Lmk if that is fine with you. I could imagine theoretical ways this doesn't work (I'm thinking some path where the prefix is actually required to be resolvable) but considering the current state this is probably the better approach

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Oh very nice! Thank you for digging into this so thoroughly. That issue is v useful for context

@lpil lpil merged commit c0b945b into gleam-lang:main Mar 30, 2024
11 checks passed
@Acepie Acepie deleted the windowsLocalTomlPath branch April 6, 2024 12:00
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.

manifest.toml uses absolute paths for relative path devs on Windows
2 participants