-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 building slnf with @ in the path #11421
base: main
Are you sure you want to change the base?
Conversation
…, str2) because it is not available for .net framework
@@ -658,7 +658,7 @@ internal static string ParseSolutionFromSolutionFilter(string solutionFilterFile | |||
JsonDocumentOptions options = new JsonDocumentOptions() { AllowTrailingCommas = true, CommentHandling = JsonCommentHandling.Skip }; | |||
JsonDocument text = JsonDocument.Parse(File.ReadAllText(solutionFilterFile), options); | |||
solution = text.RootElement.GetProperty("solution"); | |||
return FileUtilities.GetFullPath(solution.GetProperty("path").GetString(), Path.GetDirectoryName(solutionFilterFile)); | |||
return FileUtilities.GetFullPath(solution.GetProperty("path").GetString(), Path.GetDirectoryName(solutionFilterFile), escape: false); |
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.
Could this be
return FileUtilities.GetFullPath(solution.GetProperty("path").GetString(), Path.GetDirectoryName(solutionFilterFile), escape: false); | |
return FileUtilities.NormalizePath(Path.GetDirectoryName(solutionFilterFile), solution.GetProperty("path").GetString()); |
without changing GetFullPath
?
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.
this won't work because in GetFullPath before normalization the solution path that is provided in slnf needs to be fixed for the current machine.
msbuild/src/Shared/FileUtilities.cs
Line 759 in 04b6e1b
fileSpec = FixFilePath(EscapingUtilities.UnescapeAll(fileSpec)); |
I can create new function GetFullPathUnescaped and duplicate the code there. What do you think?
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.
is it possible to do here
string fullPath = NormalizePath(Path.Combine(currentDirectory, FileUtilities.FixFilePath(EscapingUtilities.UnescapeAll(fileSpec)));
instead of making modification of FileUtilities.cs
?
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.
there is also adding trailing slash at the end of the full path
msbuild/src/Shared/FileUtilities.cs
Lines 764 to 772 in aff5455
if (NativeMethodsShared.IsWindows && !EndsWithSlash(fullPath)) | |
{ | |
if (FileUtilitiesRegex.IsDrivePattern(fileSpec) || | |
FileUtilitiesRegex.IsUncPattern(fullPath)) | |
{ | |
// append trailing slash if Path.GetFullPath failed to (this happens with drive-specs and UNC shares) | |
fullPath += Path.DirectorySeparatorChar; | |
} | |
} |
Not modifying the FileUtilities.GetFullPath
by adding extra check makes sense because it is on a hot path. Adding a new method FileUtilities.GetFullPathUnescaped
that will duplicate most of the code except escaping would be a compromise for that reason.
Is there other reason not to modify FileUtilities.cs
that I miss?
Could this PR maybe also fix #11442? |
This is probably not related. The bug that this PR fixes is not recent and existed before. The one you described looks like a regression. Nevertheless, we will fix it soon. Thanks for reporting! |
hmm too bad but thank you for your time and feedback :) |
Context
Fixes #11050
If the path to the solution file contains
@
, it builds normally. But, if solution filter references this solution then the build fails:same happens with other symbols like
%
and$
.See the issue description for more details.
Details
The problem occurs on this line:
msbuild/src/Build/Construction/Solution/SolutionFile.cs
Line 661 in a1c2e74
FileUtilities.GetFullPath
changes@
to%40
.Specifically this happens here:
msbuild/src/Shared/FileUtilities.cs
Line 762 in 18c6b2e
Changes
Added
bool escape
param toFileUtilities.GetFullPath
to be able to skip escape for getting solution path from solution filter (.slnf) jsonTesting
Added new test to cover this scenario