Skip to content
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

Enable opt-in "GeneratePathProperty" metadata on a PackageReference to generate a per package MSBuild property to "Foo.Bar\1.0\" directory #6949

Closed
jeffkl opened this issue May 18, 2018 · 12 comments · Fixed by NuGet/NuGet.Client#2271

Comments

@jeffkl
Copy link
Contributor

jeffkl commented May 18, 2018

Problem Statement

Some packages' layout does not conform to the best practices. Consumers of these packages can guess the path with something like this:

<ItemGroup>
  <PackageReference Include="Foo" Version="1.0.0" />
</ItemGroup>

<PropertyGroup>
  <FooTool>$(NuGetPackageRoot)\Foo.Bar\1.0.0\tool\foo.exe"</FooTool>
</PropertyGroup>

The Foo package only has a tool folder with an EXE that I want to execute but the package author did not include any build\Foo.props that sets a property to the path of the package. So I have to derive the path based on the version. This causes some headaches when we update versions because the version value is in two places. You can use properties instead but that break Visual Studio Package Manager.

Proposal

Add a CreateProperty metadata item to <PackageReference /> which is the name of an MSBuild property to create that points to the root of the expanded package.

<ItemGroup>
  <PackageReference Include="Foo" Version="1.0.0" CreateProperty="PkgFoo" />
</ItemGroup>

The .g.props file would then contain:

  <PropertyGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <PkgFoo Condition=" '$(PkgFoo)' == '' ">C:\Users\Bob\.nuget\packages\foo\1.0.0</PkgFoo>
  </PropertyGroup>

And the above example can become:

<PropertyGroup>
  <FooTool>$(PkgFoo)\tool\foo.exe"</FooTool>
</PropertyGroup>

This would only be useful for packages that are non-standard which should be a minority. I am concerned that if we add this feature that it will just perpetuate bad packages. However, the reality is that we have countless code bases which consume packages that could benefit from this feature.

It would not work for transitive dependencies. If you wanted a property you would need to add an explicit package reference.

I am willing to contribute this and have a prototype already coded up.

@jeffkl
Copy link
Contributor Author

jeffkl commented May 18, 2018

FYI @AndyGerlicher @kingerja

@AndyGerlicher
Copy link

I think this is a really good compromise. We're seeing bigger repos try to migrate to PackageReference internally and it's causing a lot of pain for little things like this, especially when a package hasn't been updated in years and the owner has no interest in conforming. I'd heard the proposal of making every package have a property available, but I think that could lead to worse behavior by default. I like this a lot.

@rrelyea
Copy link
Contributor

rrelyea commented May 18, 2018

There is something like this that NuGet already does. But only in some cases.
Let's remember that, and understand if we should continue to enhance that existing feature … first.
I believe that may solve the same problem.
(cannot recall right now...but can dig into this soon...)

@rrelyea
Copy link
Contributor

rrelyea commented Jun 14, 2018

See previous work here: #5975

We discussed in person with @jeffkl and msbuild team...

Decided:
Given id of My.Package, if it had a tool folder, $(PackageToolDir_My_Package_Name) would be created in *.nuget.g.props
That would be true of all projects that transitive reference that package.
We'd also support a ExportPathVariable="PackageDir_MyFolder" attribute on PackageRef that would enable the user to request that a PackageDir prop, of the given name, be created.

More work needed:
Would PR project system need any changes?
Would PR nomination from SDK team flow the data correctly?

@jeffkl
Copy link
Contributor Author

jeffkl commented Jun 15, 2018

@rrelyea et al, can we close on the name of the attribute you want to use? Options we've discussed:

<PackageReference Include="PackageA" Version="1.0.0" PropertyName="PkgPackageA" />
<PackageReference Include="PackageA" Version="1.0.0" MSBuildPropertyName="PkgPackageA" />
<PackageReference Include="PackageA" Version="1.0.0" ExportProperty="PkgPackageA" />
<PackageReference Include="PackageA" Version="1.0.0" ExportMSBuildProperty="PkgPackageA" />
<PackageReference Include="PackageA" Version="1.0.0" ExportVariable="PkgPackageA" />
<PackageReference Include="PackageA" Version="1.0.0" CreateMSBuildProperty="true" />
<PropertyGroup>
  <NuGetGeneratePackageProperties>true</NuGetGeneratePackageProperties>
</PropertyGroup>

@jainaashish
Copy link
Contributor

one more option PkgPathPropertyName

@jainaashish
Copy link
Contributor

PR# NuGet/NuGet.Client#2271 has been merged to dev. Once approved, will also merge into 15.9.

@jeffkl
Copy link
Contributor Author

jeffkl commented Aug 27, 2018

@jainaashish how do we get approval for 15.9? We need to get this in ASAP but haven't figured out how you want to go about it.

@jainaashish
Copy link
Contributor

@jeffkl I'm not sure what is the whole process to get this approve for 15.9... may be @DoRonMotter can help here?

@wli3
Copy link

wli3 commented Aug 30, 2018

There should be a block on normal project PackageReference a PackageType=DotnetTool? So how could user use it in their csproj? Or all these internal package don't have the PackageType

@dasMulli
Copy link

Once approved, will also merge into 15.9.

Did this happen?

@jainaashish
Copy link
Contributor

jainaashish commented Sep 10, 2018

Yes, this has been merged for VS 15.9 with NuGet/NuGet.Client@e082b2b

@rrelyea rrelyea changed the title Generate MSBuild property that has the path to a package Enable opt-in "GeneratePathProperty" metadata on a PackageReference to generate a per package MSBuild property to "Foo.Bar\1.0\" directory Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants