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

workspace: Trim prefix canonicalize path for windows #21101

Closed

Conversation

dovakin0007
Copy link
Contributor

Release Notes:

  • N/A

@dovakin0007 dovakin0007 changed the title workspace: trim prefix canonicalize path for windows workspace: Trim prefix canonicalize path for windows Nov 23, 2024
@dovakin0007
Copy link
Contributor Author

had to trim it cause it was causing issue with powershell
the left one is 0.164 the right side one is the fixed version
image

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Nov 23, 2024

1. This PR has no description so it's not clear why are you doing this
2. It is an error to trim a UNC path over 260 chars long (https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry)
3. This is not the only place where canonicalized paths need better handling, and doing it just here is a hack, not a fix
4. #19727

@SomeoneToIgnore
Copy link
Contributor

Ok, at least we've got a reasoning behind it, great.

Does not seem a right way to go, but does indeed fix a large breakage in the recent Windows builds.
@JunkuiZhang do you have any preferences on what to do with this?

@dovakin0007
Copy link
Contributor Author

dovakin0007 commented Nov 23, 2024

1. This PR has no description so it's not clear why are you doing this 2. It is an error to trim a UNC path over 260 chars long (https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry) 3. This is not the only place where canonicalized paths need better handling, and doing it just here is a hack, not a fix 4. #19727

yeah this is hack for now cause it does causes issues while running with build.rs it didn't happen 0.161 and I understand canonicalized does need a overhaul for windows

@SomeoneToIgnore SomeoneToIgnore added the cla-signed The user has signed the Contributor License Agreement label Nov 23, 2024
@JunkuiZhang
Copy link
Contributor

Does not seem a right way to go, but does indeed fix a large breakage in the recent Windows builds.

Seems so.

do you have any preferences on what to do with this?

I was wondering if there’s a chance we could move forward with #19727? Otherwise, we might end up needing more and more hackings like this PR in the future.

I’m not sure how to express this perfectly in English, but based on the current changes in #19727, I feel it minimizes the scope of modifications as much as possible. Strictly speaking, #19727 only ensures that any paths entering Zed are sanitized. However, its potential limitation is that it doesn’t guarantee the paths interacting with these sanitized paths are also sanitized (this is explained in more detail in #19727).

To address this further, we might need to make more extensive changes, similar to what was done in #19228.

@SomeoneToIgnore
Copy link
Contributor

I fully agree to move on with the minimum surface sanitized approach you have (fixing potentially missed places later), just was not sure what is needed to move it forward, esp. after @ConradIrwin brought more ideas into the discussion in another PR.

Since Windows builds are badly broken right now (my opinion), I feel that anything that does not look like a bad hack (as here) is a good start already.

Let me gather my thoughts on Monday and consider the merge.

Meanwhile, would be great to get comment with a recap on what's left to push that draft sanitization PR to completion.

@JunkuiZhang
Copy link
Contributor

I fully agree to move on with the minimum surface sanitized approach you have (fixing potentially missed places later), just was not sure what is needed to move it forward, esp. after @ConradIrwin brought more ideas into the discussion in another PR.

Since Windows builds are badly broken right now (my opinion), I feel that anything that does not look like a bad hack (as here) is a good start already.

I see, this PR looks good to me.

Let me gather my thoughts on Monday and consider the merge.

Thanks for your efforts on pushing Windows forward!

Meanwhile, would be great to get comment with a recap on what's left to push that draft sanitization PR to completion.

I’ll add comments in #19727 tomorrow.

@SomeoneToIgnore
Copy link
Contributor

We can always get back to this after considering #19727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants