Fix relative path handling on Windows. by peterhuene · Pull Request #7842 · dotnet/cli

peterhuene · 2017-10-16T06:50:15Z

On Windows, PathUtility.GetRelativePath was not properly handling
paths that differed by case in the drive reference (e.g. "C:" vs.
"c:"). The fix was to add the missing case-insensitive comparison
argument.

Replaced uses of PathUtility.GetRelativePath with
Path.GetRelativePath where possible (requires 2.0.0+).

Additionally, PathUtility.RemoveExtraPathSeparators was not handling
paths with drive references on Windows. If the path contained a drive
reference, the separator between the drive reference and the first part
of the path was removed. This is due to Path.Combine not handling
this case, so an explicit concatenation of the separator was added.

This commit resolves issue #7699.

dnfclas · 2017-10-16T06:50:19Z

@peterhuene,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

dnfclas · 2017-10-16T06:57:56Z

@peterhuene, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

nguerrera · 2017-10-18T23:47:05Z

I haven't reviewed this in detail yet, but my first thought is whether we can now make use of dotnet/corefx#11689 cc @JeremyKuhne

peterhuene · 2017-10-19T00:12:50Z

That would be great to remove this helper in favor of a framework method.

JeremyKuhne · 2017-10-19T00:52:37Z

Definitely would be good to use the framework method or at least copy the code if you can't for some reason. If you find any issues with the algorithm let me know.

peterhuene · 2017-10-19T00:54:07Z

I'll amend the commit with the proposed change.

peterhuene · 2017-10-23T17:55:02Z

@JeremyKuhne there were two places in the codebase I wasn't able to move to Path.GetRelativePath: build_projects/dotnet-cli-build/MakeRelative.cs, which has its own copy of GetRelativePath (now fixed too), and test/Microsoft.DotNet.TestFramework/TestAssetInstance.cs; both are due to targeting an older framework.

The places where I could do so, I replaced with calls to Path.GetRelativePath. Ideally I'd like to get rid of the utility method entirely, but I'm not comfortable bumping framework references given my relatively limited experience with the project. Given that the one place where we now use it is in the test assembly, is it worth copying the implementation from the framework to PathUtility? I'd be happy to do so if we feel the need.

JeremyKuhne · 2017-10-23T18:06:52Z

I'm not comfortable bumping framework references

@nguerrera any guidance on this one?

nguerrera · 2017-10-23T19:57:47Z

Hmm, as far as I can tell, dotnet-cli-build should work fine since it targets $(CliTargetFramework) == netcoreapp2.1. Also, We probably don't even need a MakeRelative build task because msbuild has https://msdn.microsoft.com/en-us/library/dd633440.aspx#BKMK_MakeRelative there instead.)

TestFramework is more interesting because it's netstandard and there's no netstandard version with Path.GetRelativePath. I'm looking at the code to understand if the tests really need this or if we can achieve what it's doing in a simpler way.

nguerrera · 2017-10-23T20:10:41Z

Oh, I see MakeRelative task is a workaround for a bug in MSBuild::MakeRelative on x-plat, so never mind that change.

I'm still not seeing why we can't use Path.GetRelativePath in dotnet-cli-build. What is the error when you try to do that?

We can leave the TestFramework usage alone and your fix in Cli.Utils. We're going to be trimming down Cli.Utils at some point soon and cleaning that up can be part of that.

peterhuene · 2017-10-23T20:17:17Z

MakeRelative.cs(32,37): error CS0117: 'Path' does not contain a definition for 'GetRelativePath' [...\cli\build_projects\dotnet-cli-build\dotnet-cli-build.csproj] [...\cli\build.proj]

I see it getting built for netstandard1.6 in my environment.

nguerrera · 2017-10-23T20:52:06Z

Looking deeper, it's oddly getting built for netcoreapp2.1, but without an appropriately recent reference to Microsoft.NETCore.App. I think some build refactoring caused this project to inherit DisableImplicitFrameworkReferences=true.

If you add the following to dotnet-cli-build.csproj (like other TargetFramework=$(CliTargetFramework) in the tree), it should work:

    <PackageReference Include="Microsoft.NETCore.App" Version="$(CLI_SharedFrameworkVersion)" />

peterhuene · 2017-10-23T21:49:13Z

I'll modify the project and see if I can get it building; as Path.GetRelativePath doesn't support passing a custom directory separator, we'll have to make a breaking change to this task to remove the SeparatorChar property (or otherwise make it obsolete); I do not see this property used anywhere in the codebase though.

On Windows, `PathUtility.GetRelativePath` was not properly handling paths that differed by case in the drive reference (e.g. "C:\" vs. "c:\"). The fix was to add the missing case-insensitive comparison argument. Replaced uses of `PathUtility.GetRelativePath` with `Path.GetRelativePath` where possible (requires 2.0.0+). Additionally, `PathUtility.RemoveExtraPathSeparators` was not handling paths with drive references on Windows. If the path contained a drive reference, the separator between the drive reference and the first part of the path was removed. This is due to `Path.Combine` not handling this case, so an explicit concatenation of the separator was added. This commit resolves issue #7699.

peterhuene · 2017-10-24T18:24:35Z

@nguerrera pushed the changes to the task.

Sign in to view


            var relativePathReferences = _appliedCommand.Arguments.Select((r) =>
-                                                                              PathUtility.GetRelativePath(msbuildProj.ProjectDirectory, Path.GetFullPath(r)))
+                                                                              Path.GetRelativePath(msbuildProj.ProjectDirectory, Path.GetFullPath(r)))


Sign in to view

-            var relativePath = GetRelativePath(Path1, Path2, SeparatorChar);
-
-            RelativePath = ToTaskItem(Path1, Path2, relativePath);
+            RelativePath = ToTaskItem(Path1, Path2, Path.GetRelativePath(Path1, Path2));


dnfclas added the cla-required label Oct 16, 2017

dnfclas added cla-signed and removed cla-required labels Oct 16, 2017

livarcocc reviewed Oct 24, 2017

View reviewed changes

nguerrera approved these changes Oct 24, 2017

View reviewed changes

livarcocc approved these changes Oct 24, 2017

View reviewed changes

livarcocc merged commit 1e70555 into dotnet:master Oct 27, 2017

peterhuene deleted the fix-7699 branch December 8, 2017 20:34

Conversation

peterhuene commented Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Oct 16, 2017

Uh oh!

dnfclas commented Oct 16, 2017

Uh oh!

nguerrera commented Oct 18, 2017

Uh oh!

peterhuene commented Oct 19, 2017

Uh oh!

JeremyKuhne commented Oct 19, 2017

Uh oh!

peterhuene commented Oct 19, 2017

Uh oh!

peterhuene commented Oct 23, 2017

Uh oh!

JeremyKuhne commented Oct 23, 2017

Uh oh!

nguerrera commented Oct 23, 2017

Uh oh!

nguerrera commented Oct 23, 2017

Uh oh!

peterhuene commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguerrera commented Oct 23, 2017

Uh oh!

peterhuene commented Oct 23, 2017

Uh oh!

peterhuene commented Oct 24, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

peterhuene commented Oct 16, 2017 •

edited

Loading

peterhuene commented Oct 23, 2017 •

edited

Loading