Add Path.GetRelativePath by JeremyKuhne · Pull Request #11689 · dotnet/corefx

JeremyKuhne · 2016-09-13T21:56:09Z

This change adds Path.GetRelativePath, which will calculate the relative
path from one directory to another file/directory.

The original proposal: #6191

@terrajobst, @ericstj, @jamesqo
CC: @danmosemsft

davidfowl · 2016-09-13T22:00:26Z

cc @BrennanConroy

jamesqo · 2016-09-13T22:04:31Z

+
+        /// <summary>
+        /// Create a relative path from one path to another. Paths will be resolved before calculating the difference.
+        /// Default path comparison for the active platform will be used (OrdinalIgnoreCase for Windows, Ordinal for Unix/Mac).


/Mac is not necessary.

stephentoub · 2016-09-14T11:48:54Z


+        internal const string ParentDirectoryPrefix = @"../";
+
+        internal static readonly StringComparison DefaultPathComparison = StringComparison.Ordinal;


Shouldn't this be OrdinalIgnoreCase on macOS?

As I noted yesterday, we also already have internal support for this, which we should just reuse:
https://github.com/dotnet/corefx/blob/master/src/Common/src/System/IO/PathInternal.CaseSensitivity.cs

Shouldn't this be OrdinalIgnoreCase on macOS?

Yes, thanks.

As I noted yesterday, we also already have internal support for this

I'm slightly uncomfortable with case sensitivity coming from the state of the temp directory. I need to think that over a bit.

Unfortunately I can't use the existing class as it has dependencies on System.IO.FileSystem. Updating with comments.

JeremyKuhne · 2016-09-15T01:58:17Z

@joperezr This should tackle #11321 - can you take a look?

JeremyKuhne · 2016-09-15T02:00:19Z

+
+:: Need updated nuget binaries in our cli drop
+echo Updating CLI Nuget Frameworks map...
+robocopy "%TOOLRUNTIME_DIR%" "%TOOLRUNTIME_DIR%\dotnetcli\sdk\%DOTNET_VERSION%" Nuget.Frameworks.dll /XO >> "%INIT_TOOLS_LOG%"


Note that copying all of the Nuget.* dlls causes failures.

I was expecting Nuget.*.dll to work as well. What errors are you seeing?

In reply to: 78878113 [](ancestors = 78878113)

It tries to bring in some other class that it can't find- I didn't dig into it further, just pedaled back for expediency

Why are we doing this as opposed to pulling them in via BuildTools?

They are already in buildtools, we're just making CLI use the updated one from buildtools. You could ask "why aren't we pulling it in via CLI?" and the answer would be: CLI takes too long to update and its unclear if folks care about updating the current form given the msbuild switch.

JeremyKuhne · 2016-09-15T02:06:41Z

@dotnet-bot test Innerloop Windows_NT Debug Build and Test

JeremyKuhne · 2016-09-15T02:19:19Z

Not sure why the init-tools.cmd is failing on the CI, is there a way to get init-tools.log files?

ericstj · 2016-09-15T16:07:07Z

@dotnet-bot test Innerloop Windows_NT Debug Build and Test

joperezr · 2016-09-15T17:31:20Z

 echo %PROJECT_JSON_CONTENTS% > "%PROJECT_JSON_FILE%"
 echo Running %0 > "%INIT_TOOLS_LOG%"

+set /p DOTNET_VERSION=< "%~dp0DotnetCLIVersion.txt"


Why moving this line?

I need it outside of the block that might be skipped

joperezr · 2016-09-15T17:31:38Z

 :afterbuildtoolsrestore

-echo Initializing BuildTools ...
+echo Initializing BuildTools...


Nit: Unwanted space

I removed the extra space to make it match the rest

joperezr · 2016-09-15T17:33:54Z

    fi
+
+    echo "Updating CLI Nuget Frameworks map..."
+    cp -u $__TOOLRUNTIME_DIR/Nuget.Frameworks.dll $__TOOLRUNTIME_DIR/dotnetcli/sdk/$__DOTNET_TOOLS_VERSION >> $__init_tools_log


Should we verify that the exit code is 0 after this?

Probably- I need to look up how to do this, so rusty with bash...

that is what the previous checks for if [ "$?" != "0" ]; then checks are doing

joperezr · 2016-09-15T17:35:30Z

  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <ItemGroup>
-    <Project Include="*\ref\**\*.*proj">
+    <Project Include="*\ref\**\*.builds;*\ref\**\*.*proj">


I believe that with this approach some projects will get built twice? wherever there is a builds file, we shouldn't build the csprojs that it calls, but I'm not sure how to do this. @ericstj thoughts?

It won't actually build twice, MSBuild is smart enough to prevent that. If this becomes common-place we'll switch to builds everywhere, but right now that's unnecessary overhead.

I also worry about this a little but I'm good with this for now.

joperezr · 2016-09-15T17:36:49Z

+  <ItemGroup>
+    <Project Include="System.Runtime.Extensions.csproj" />
+    <Project Include="System.Runtime.Extensions.csproj">
+      <TargetGroup>netcoreapp1.1</TargetGroup>


@ericstj should we have a different AssemblyVersion for the netcoreapp1.1 contract?

No: we have similar precedent on desktop where more surface area is visible via facades.

joperezr · 2016-09-15T17:37:51Z

@@ -0,0 +1 @@
+MembersMustExist : Member 'System.IO.Path.CreateRelativePath(System.String, System.String)' does not exist in the implementation but it does exist in the contract.


How come this wasn't complaining before?

We can't have exceptions for net462, that's already shipped. I suspect that the net462 targetgroup is resolving the wrong contract assembly.

I'll try removing this- it might be an artifact of iteration

joperezr · 2016-09-15T17:40:08Z

+  <ItemGroup Condition="'$(TargetGroup)'=='netcoreapp1.1'">
    <TestNugetTargetMoniker Include="$(NugetTargetMoniker)">
      <Folder>netcoreapp1.1</Folder>
    </TestNugetTargetMoniker>


You don't need this as I saw that it is already in dir.props

joperezr · 2016-09-15T17:40:49Z

  <ItemGroup Condition="'$(TargetGroup)'=='netstandard1.7'">
+    <TestNugetTargetMoniker Include="$(NugetTargetMoniker)">
+      <Folder>ns1.7-netcoreapp1.1</Folder>
+    </TestNugetTargetMoniker>


This should probably go in dir.props as I suppose a lot of test projects will start doing this.

ericstj · 2016-09-16T16:05:54Z

@JeremyKuhne , can you rebase on master and remove the init-tools workaround?

JeremyKuhne · 2016-09-16T18:43:17Z

@ericstj I removed the init-tools workaround last night.

JeremyKuhne · 2016-09-16T23:16:20Z

@ericstj, @weshaggard, @joperezr This should be clean now- can I get sign off?

ericstj · 2016-09-16T23:41:48Z

+    "System.Runtime": "4.1.0"
  },
  "frameworks": {
    "netstandard1.7": {


This will generate a warning when you build the netcoreapp1.1 configuration. You can also delete the imports, they're no longer needed for the package version you are referencing.

Thanks, fixed

ericstj

Modulo a couple nits it looks good.

ericstj · 2016-09-16T23:42:48Z

    <TargetingPackReference Include="System.Private.Interop" />
-    <TargetingPackReference Include="System.Private.Reflection" /> <!-- can remove once we remove the Environment reflection workaround -->
+    <TargetingPackReference Include="System.Private.Reflection" />
+    <!-- can remove once we remove the Environment reflection workaround -->


nit: reformatting here may have changed meaning of the comment.

Moved this above, thanks

JeremyKuhne · 2016-09-17T03:24:05Z

@dotnet-bot test Innerloop Windows_NT Release Build and Test

This change adds Path.GetRelativePath, which will calculate the relative path from one directory to another file/directory. This change also adds support for targetting netcoreapp

JeremyKuhne · 2016-09-19T20:56:10Z

@dotnet-bot test this please

joperezr · 2016-09-19T21:25:32Z

 {
  "dependencies": {
-    "System.Runtime": "4.0.20"
+    "System.Runtime": "4.1.0"


Can we change this to the latest System.Runtime package prerelease? that way you will get netstandard1.7 version of System.Runtime surface area.

Sure, I'll update it

joperezr · 2016-09-19T21:29:02Z

-    "netstandard1.7": {}
+    "netstandard1.7": {},
+    "netcoreapp1.1": {},
+    "netcoreapp1.0": {}


you shouldn't need netcoreapp1.0 since that should come from the supports corefx.Test.netcoreapp1.0

Ok, I'll try and remove it

joperezr · 2016-09-19T21:31:02Z

    <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.7'">$(DefineConstants);netstandard17</DefineConstants>
    <NugetTargetMoniker Condition="'$(NugetTargetMoniker)'==''">.NETStandard,Version=v1.5</NugetTargetMoniker>
  </PropertyGroup>
+  <ItemGroup Condition="'$(TargetGroup)'=='netstandard1.7'">


We should log an issue in buildtools to move this code there instead.

This should go with the 1.2 issue, right? Or at least be associated.

joperezr

Couple of comments but this LGTM assuming CI is green.

The workarounds are no longer needed and fail with the dir.targets workaround removed.

Test targets need updated to avoid issues when building multithreaded. See #11468.

JeremyKuhne · 2016-09-20T04:22:22Z

@dotnet-bot test Innerloop CentOS7.1 Release Build and Test

JeremyKuhne · 2017-08-21T18:09:48Z

We want to get this in netfx eventually.

…ePath Add Path.GetRelativePath Commit migrated from dotnet/corefx@e4ea1fd

dnfclas added the cla-already-signed label Sep 13, 2016

jamesqo reviewed Sep 13, 2016
View reviewed changes

JeremyKuhne force-pushed the CreateRelativePath branch 2 times, most recently from a08e1d5 to d7b445c Compare September 14, 2016 04:16

stephentoub reviewed Sep 14, 2016
View reviewed changes

JeremyKuhne force-pushed the CreateRelativePath branch 2 times, most recently from 4fdae51 to c50d2c3 Compare September 15, 2016 01:56

JeremyKuhne commented Sep 15, 2016

View reviewed changes

joperezr reviewed Sep 15, 2016

View reviewed changes

JeremyKuhne force-pushed the CreateRelativePath branch 2 times, most recently from c3f4c5b to bc045b5 Compare September 16, 2016 23:15

ericstj reviewed Sep 16, 2016

View reviewed changes

ericstj approved these changes Sep 16, 2016

View reviewed changes

JeremyKuhne force-pushed the CreateRelativePath branch from bc045b5 to b8c9a4e Compare September 17, 2016 00:16

Add Path.GetRelativePath

387cf98

This change adds Path.GetRelativePath, which will calculate the relative path from one directory to another file/directory. This change also adds support for targetting netcoreapp

JeremyKuhne force-pushed the CreateRelativePath branch from b8c9a4e to 387cf98 Compare September 19, 2016 18:21

joperezr reviewed Sep 19, 2016

View reviewed changes

joperezr approved these changes Sep 19, 2016

View reviewed changes

JeremyKuhne added 2 commits September 19, 2016 15:36

Address feedback, remove workarounds

1b02003

The workarounds are no longer needed and fail with the dir.targets workaround removed.

Disable ns 1.7 test build

4e7e421

Test targets need updated to avoid issues when building multithreaded. See #11468.

weshaggard approved these changes Sep 20, 2016

View reviewed changes

JeremyKuhne merged commit e4ea1fd into dotnet:master Sep 20, 2016

hughbe mentioned this pull request Sep 25, 2016

Add new Queue and Stack APIs #12047

Merged

karelz modified the milestone: 1.2.0 Dec 3, 2016

JeremyKuhne deleted the CreateRelativePath branch January 3, 2017 21:28

rainersigwald mentioned this pull request Feb 3, 2017

[MSBuild]::MakeRelative fails with Unix paths dotnet/msbuild#429

Closed

JeremyKuhne added the netfx-port-consider label Aug 21, 2017

nguerrera mentioned this pull request Oct 18, 2017

Fix relative path handling on Windows. dotnet/cli#7842

Merged

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#11689 from JeremyKuhne/CreateRelativ…

dc951a2

…ePath Add Path.GetRelativePath Commit migrated from dotnet/corefx@e4ea1fd


		internal const string ParentDirectoryPrefix = @"../";

		internal static readonly StringComparison DefaultPathComparison = StringComparison.Ordinal;

		@@ -0,0 +1 @@
		MembersMustExist : Member 'System.IO.Path.CreateRelativePath(System.String, System.String)' does not exist in the implementation but it does exist in the contract. No newline at end of file

Conversation

JeremyKuhne commented Sep 13, 2016

Uh oh!

davidfowl commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne commented Sep 15, 2016

Uh oh!

JeremyKuhne commented Sep 15, 2016

Uh oh!

ericstj commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj commented Sep 16, 2016

Uh oh!

JeremyKuhne commented Sep 16, 2016

Uh oh!

JeremyKuhne commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment