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

Handle unsupported paths in ProjectInSolution.AbsolutePath #6238

Merged
merged 3 commits into from
Mar 12, 2021

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Mar 10, 2021

Fixes #6236

Context

#5950 introduced a call to Path.GetFullPath() to get a path that is normalized. However, in some cases Visual Studio stores unsupported paths which will cause Path.GetFullPath() to throw.

Path.GetFullPath() can also throw a PathTooLongException which is now handled.

Changes Made

Revert to legacy code path of just calling Path.Combine() then do the Path.GetFullPath() in a try...catch on a best effort.

Testing

Regression test added

Notes

https://developercommunity.visualstudio.com/t/msbuild-failing-with-web-site-project-in/1359528

dotnet#5950 introduced a call to `Path.GetFullPath()` to get a path that is normalized.  However, in some cases Visual Studio stores unsupported paths which will cause `Path.GetFullPath()` to throw.

`Path.GetFullPath()` can also throw a `PathTooLongException` which is now handled.

Fixes dotnet#6236
@benvillalobos
Copy link
Member

Copy/pasting my comment from the issue:

@jeffkl looking at dates for 16.9 servicing releases. Looks like final sign off for 16.9.2 is in five days, and sign off for 16.9.3 is 4/12.
I'm open to either bringing in the quick fix before 16.9.2 and then implementing the long-term solution.
Or going straight for the long term solution.

If this code path really doesn't get hit much during builds (particularly web builds), I'm open to just catching the exception and leaving it at that.

@benvillalobos
Copy link
Member

Whatever we decide, this should target vs16.9 and flow back into master

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Looks mostly good!

}
catch (Exception)
{
// The call to GetFullPath can throw if the relative path is a URL or the paths are too long for the current file system
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a simpler path normalization here like (look for \..\) -> remove along with the prior path segment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd hate to get the logic wrong, I think the new code is pretty good to handle the situation more correctly.

@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 10, 2021

Wow that's interesting, on .NET Core Path.GetFullPath() doesn't throw, it just mangles the URL

Path.GetFullPath(@"C:\Users\jeffkl\AppData\Local\Temp\srl1rctk.qyt\http://localhost:8080")
C:\Users\jeffkl\AppData\Local\Temp\srl1rctk.qyt\http:\localhost:8080

I'll have to call Uri.TryCreate()...

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Mar 12, 2021
@benvillalobos benvillalobos merged commit f137b70 into dotnet:master Mar 12, 2021
jeffkl added a commit to jeffkl/msbuild that referenced this pull request Mar 16, 2021
marcpopMSFT added a commit that referenced this pull request Mar 17, 2021
Handle unsupported paths in ProjectInSolution.AbsolutePath (#6238)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken build for solutions with web site projects
3 participants