-
-
Notifications
You must be signed in to change notification settings - Fork 757
GH4478: Add ExpandShortPath to resolve Windows short paths #4500
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
GH4478: Add ExpandShortPath to resolve Windows short paths #4500
Conversation
|
Added unit tests to ensure the changes are correct (and it cannot be broken in the future). |
|
@devlead anything I can do (to help) to get this merged into develop so I can start using the improved caching behavior? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After internal discussion, we're a bit hesitant to change the core Path classes to introduce new implicit behavior, especially given this is addressing a platform-specific edge case. We'd prefer to avoid altering base logic unless there's a broader need.
As an alternative, could you introduce a helper method in PathExtensions for this case? This could then be used i.e. in ScriptRunner.GetToolPath / GetToolPath.
|
What would be a good name for such an extension method? |
Go with |
|
PR is updated. |
- Add ExpandShortPath extension methods for FilePath and DirectoryPath - Expand short paths (e.g. PROGRA~1) to full paths for proper tool/addin location - Apply short path expansion in ScriptProcessor for installation paths - Add comprehensive tests for new functionality - fixes cake-build#4478
c5ee5b0 to
4e4013c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
|
@GeertvanHorrik your changes have been merged, thanks for your contribution 👍 |
Fixes #4478
On Windows, when running from PowerShell terminal, the %temp% (and other) environment variables are expanded to a short path, causing issues when installing addins (NuGet can't find the files).
This PR resolves the short to long paths in the Path.
Added unit tests and tested this myself.
Other options I considered are when parsing the config file (when expanding), but at this stage Cake is not aware whether it's a path or not, and using path conversions can lead to unwanted side effects.