Fix MakeRelative regression in v16.10 (rebased to vs16.10 branch)#6513
Merged
rainersigwald merged 2 commits intodotnet:vs16.10from Jun 8, 2021
Merged
Fix MakeRelative regression in v16.10 (rebased to vs16.10 branch)#6513rainersigwald merged 2 commits intodotnet:vs16.10from
rainersigwald merged 2 commits intodotnet:vs16.10from
Conversation
Added some more unit tests as per suggestions Fixed unit test failing on linux Removed unnecessary length check Co-authored-by: Forgind <Forgind@users.noreply.github.com>
Contributor
|
The MSBuild team agreed we would try to take this for 16.10 as you suggested. We will need internal approval, so I'm modifying your opening message to fit the template. |
danmoseley
reviewed
Jun 4, 2021
| } | ||
| sb.Length--; | ||
|
|
||
| if (fullPath[fullPath.Length - 1] != Path.DirectorySeparatorChar) |
Member
There was a problem hiding this comment.
should it also check Path.AlternateDirectorySeparatorChar?
Author
There was a problem hiding this comment.
My understanding is that the fullPath has already been normalised within the Path.GetFullPath function so should only contain Path.DirectorySeparatorChar?
Contributor
Author
There was a problem hiding this comment.
Sure is:
Microsoft (R) Visual C# Interactive Compiler version 3.10.0-4.21269.26 ()
Loading context from 'CSharpInteractive.rsp'.
Type "#help" for more information.
> System.Console.WriteLine(System.IO.Path.GetFullPath("C:/abc/def/"));
C:\abc\def\
> System.Console.WriteLine(System.IO.Path.GetFullPath("C:/abc/def"));
C:\abc\def
> 
Forgind
approved these changes
Jun 7, 2021
Contributor
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
Thank you for fixing this!
rainersigwald
approved these changes
Jun 8, 2021
Comment on lines
+1571
to
+1572
| public static bool operator ==(Microsoft.Build.Graph.GraphBuildOptions left, Microsoft.Build.Graph.GraphBuildOptions right) { throw null; } | ||
| public static bool operator !=(Microsoft.Build.Graph.GraphBuildOptions left, Microsoft.Build.Graph.GraphBuildOptions right) { throw null; } |
Member
There was a problem hiding this comment.
Sorry about this; we fixed if for 16.11 but not 16.10.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6493
Summary
MSBuild's MakeRelative function does not fit the spec for how that function should work.
Customer impact
Customers who use our MakeRelative function expect the relative path produced to end in a slash if and only if the input path ends in a slash. It currently does not end in a slash whether or not the input path ended in a slash. Users can invoke this function directly, and it can lead to dlls not being found, among other problems.
Regression?
Yes, from #6311 (16.10 Preview 2). The URI-based function that had existed prior failed in another scenario, and the new form is also faster. This PR resolves a bug associated with that change.
Changes Made
Only removes the ending slash if the input path did not end in a slash.
Testing
Extensive unit test coverage. This is a community contribution, and I imagine it was also tested locally. (Please correct me if I'm wrong, @dsparkplug.)
Risk
Low. Rather than always removing the final slash, this conditionally removes it, reverting to prior behavior. No other (non-test) parts of the code base were altered.