GeneratePathProperty properties should end with a trailing slash

[rainersigwald]

Details about Problem

NuGet product used (NuGet.exe | VS UI | Package Manager Console | dotnet.exe): msbuild -t:Restore

NuGet version (x.x.x.xxx): 5.5.0-preview.1.6319+ba2a72ac9afd9e7260798a6ad14088297570b350.ba2a72ac9afd9e7260798a6ad14088297570b350

Worked before? If so, with which NuGet version: no

Detailed repro steps so we can see the same problem

When a package is referenced with GeneratePathProperty="true", a property based on the package name is created in .nuget.g.props. The value of the property is the path to the root of the expanded package on disk.

Currently, this looks like

  <PropertyGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <PkgEnterpriseLibrary_TransientFaultHandling_Core Condition=" '$(PkgEnterpriseLibrary_TransientFaultHandling_Core)' == '' ">C:\Users\raines\.nuget\packages\enterpriselibrary.transientfaulthandling.core\1.0.0</PkgEnterpriseLibrary_TransientFaultHandling_Core>
  </PropertyGroup>

Ideally this would end with a path-separator slash. That makes it easier to avoid referencing the property with a glob like $(PkgWhatever)\** which expands to {root of disk}\** which causes MSBuild to read every file on the disk, with possibly destructive effects. If the property is undefined $(PkgWhateverWithTrailingSlash)** is at least restricted to the project directory.

Various properties in common targets follow this convention for this reason.

Other suggested things

An obvious question is "should MSBuild allow this?". It's hard to definitively say "no", but we're considering logging and some other options: dotnet/msbuild#3642 dotnet/msbuild#3204

[donnie-msft]

@jeffkl can you provide some input on this one?

[jeffkl]

Knowing that the paths are always directories, it would fine to add the trailing slash. It probably should have had it from the beginning.

[nkolev92]

This would be a breaking change then, correct?
How have we handled migrations of similar behavior before?

[jeffkl]

I don't believe it would be a breaking change. If people consumed the properties like:

<PropertyGroup>
  <MyNewProperty>$(Pkg_SomePackage)\somepath</MyNewProperty>
</PropertyGroup>

Then they'd end up with an extra slash which shouldn't cause any issues, even if they used relative paths like $(Pkg_SomePackage)\..\subdir. At least on Windows, the extra slash is ignored.

[nkolev92]

At least on Windows, the extra slash is ignored.

Key point :)

[jeffkl]

I have verified that on linux, the double directory separated is also ignored

jeff@JEFFKL-DEV:/$ ls home//jeff//dotnet-install.sh
home//jeff//dotnet-install.sh

jeff@JEFFKL-DEV:/$ cat /etc/os-release
NAME="Ubuntu"
VERSION="18.04.2 LTS (Bionic Beaver)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 18.04.2 LTS"
VERSION_ID="18.04"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=bionic
UBUNTU_CODENAME=bionic

[zkat]

This seems small enough that we can probably keep in .NET 6. /cc @aortiz-msft because we might need scheduling for it, if it's going to stay in the epic

[zkat]

Pushing this into #10846 but we'll likely get it done before then. It just won't go into net6.0 GA, most likely.

[JonDouglas]

Apologies. Using a beta project that apparently updates titles on a keystroke.

[aortiz-msft]

@jeffkl - Would you mind fixing this sprint as part of Quality Week?

[jeffkl]

@aortiz-msft sure thing: NuGet/NuGet.Client#4384

[Forgind]

Turns out this was a breaking change:
https://developercommunity.visualstudio.com/t/Robocopy-command-exited-with-code--1/1690181#T-ND10008741

If an argument is supposed to be a folder, and you pass it quoted, the \" is treated as an escaped quote instead of the end of the path, and you get errors.

I'm sure there are other tools that do something similar.

[jeffkl]

We are going to revert NuGet/NuGet.Client#4384 since it turned into a breaking change (#11737)

[Nirmal4G]

Can't it be opt-in or opt-out for the time being?

[Forgind]

Can't it be opt-in or opt-out for the time being?

Up to NuGet, but in my opinion, that would just make it more confusing. You'd have to make it opt-in or opt-out when the property is set but use it (without getting to opt-in or -out) in other files, so if you don't want any extra slashes, you might have something like:

...$(PkgMy_Package)\rest_of_path
...$(PkgMy_Other_Package)rest_of_path

People seeing just one line would often get it wrong if they copy/paste...

[Nirmal4G]

So, if it has to be a breaking change, then better do it for all built-in/generated properties, I guess. This can go so wrong in so many ways 🤯 that I can't begin to describe it, if not done properly. MSBuild itself is resilient (almost) but not the external commands we pass the properties to. This is tricky!