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

'GeneratePathProperty' does not work on 'PackageDownload' #8476

Open
sharwell opened this issue Aug 21, 2019 · 10 comments
Open

'GeneratePathProperty' does not work on 'PackageDownload' #8476

sharwell opened this issue Aug 21, 2019 · 10 comments
Labels
Category:SeasonOfGiving https://devblogs.microsoft.com/nuget/nuget-season-of-giving/#season-of-giving Functionality:Restore Priority:2 Issues for the current backlog. Style:PackageReference Type:Feature

Comments

@sharwell
Copy link

The new <PackageDownload> elements do not support the GeneratePathProperty attribute, making it difficult to download a package whose contents will be used by an unrelated build task.

@nkolev92
Copy link
Member

I'd be open to adding this eventually.
The thing is PackageDownload allows duplicate ids, so we'd need a different naming schema compared to PackageReference.

Note that because of the exactness of PackageDownload (specifically, only exact versions allowed), you can create the path fairly accurately as it will be:
$(NuGetPackageRoot)\$(PackageId)\$(PackageVersion)

@nkolev92 nkolev92 added this to the Backlog milestone Aug 21, 2019
@nkolev92 nkolev92 added Priority:2 Issues for the current backlog. and removed Triage:NeedsTriageDiscussion labels Apr 27, 2020
Forgind added a commit to Forgind/msbuild that referenced this issue May 12, 2020
GeneratePathProperty does not work as seen in NuGet/Home#8476. Use workaround.
Forgind added a commit to Forgind/msbuild that referenced this issue May 21, 2020
GeneratePathProperty does not work as seen in NuGet/Home#8476. Use workaround.
Forgind added a commit to Forgind/msbuild that referenced this issue May 26, 2020
GeneratePathProperty does not work as seen in NuGet/Home#8476. Use workaround.
@nkolev92
Copy link
Member

nkolev92 commented Jan 15, 2022

@sandyarmstrong @KirillOsenkov

PackageDownload allows multiple versions, so we'd need a conflict resolution for that.

PkgDownloadCleanedUpPackageName_1 or something like that?

We'd also need to be different from PackageReference, so we'd need something to make them slightly different.

@sandyarmstrong
Copy link

sandyarmstrong commented Jan 15, 2022

How about GeneratePathProperty to get the directory path like in PackageReference, and GenerateNupkgPathProperty to get the nupkg, since it's a bit annoying to construct.

The resultant property names could just include the version. PkgSome_Id_1_0_0 for the directory, and PkgSome_Id_1_0_0_nupkg for the nupkg.

Am I missing anything that would make this problematic?

@KirillOsenkov
Copy link

One downside is when the version is changed you'd need to rename all properties.

We could have a heuristic that if only a single version is specified (the 90% case), the behavior should be identical to PackageReference (or maybe use a different prefix such as Package if it has to be separate from PackageReference convention).

I'd like to not have to mention the version in the common case.

@rainersigwald
Copy link

Prior art in CoreXT is that it defines multiple levels of the property:

    <PkgSystem_Memory_4_5_4_0>S:\NugetCache\System.Memory.4.5.4</PkgSystem_Memory_4_5_4_0>
    <PkgSystem_Memory_4_5_4>S:\NugetCache\System.Memory.4.5.4</PkgSystem_Memory_4_5_4>
    <PkgSystem_Memory_4_5>S:\NugetCache\System.Memory.4.5.4</PkgSystem_Memory_4_5>
    <PkgSystem_Memory_4>S:\NugetCache\System.Memory.4.5.4</PkgSystem_Memory_4>
    <PkgSystem_Memory>S:\NugetCache\System.Memory.4.5.4</PkgSystem_Memory>

Where I believe the highest version wins for each level. I don't know what it does for multiple prereleases in the same

Another option would be to punt to the user by taking metadata on the PackageDownload item

<PackageDownload Include="PackageName" Version="1.2.3" PathProperty="PackageName_Current" />
<PackageDownload Include="PackageName" Version="1.4.0-beta" PathProperty="PackageName_Experimental" />

@sandyarmstrong
Copy link

Another option would be to punt to the user by taking metadata on the PackageDownload item

<PackageDownload Include="PackageName" Version="1.2.3" PathProperty="PackageName_Current" />
<PackageDownload Include="PackageName" Version="1.4.0-beta" PathProperty="PackageName_Experimental" />

Yeah this is also a fine option.

@nkolev92
Copy link
Member

Unfortunately the multiple includes with the same name version does not work quite like that.

Due to a CPS limitation, PackageDownload is only 1 item with ; separated version.

You'd need to specify the property with ; as well.

I think there's also value to keeping the functionality similar, keeping GeneratePathProperty.

@olegd-superoffice
Copy link

@nkolev92 If multiple versions are specified in PackageDownload using ; to separate versions, maybe property generated with GeneratePathProperty could include multiple paths using ; to separate paths in the same order?.

@KirillOsenkov
Copy link

This is still painful. My expectation is that the simplest case works as I expect it:

<PackageDownload Include="ILRepack.Lib.MSBuild.Task" Version="[2.0.18.1]" GeneratePathProperty="true" />

@ericstj
Copy link

ericstj commented Sep 12, 2024

While we're doing this, it would pair nicely with making PackageDownload work with Central Package Management. That would alleviate the need for the project using this to manager the version or path at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:SeasonOfGiving https://devblogs.microsoft.com/nuget/nuget-season-of-giving/#season-of-giving Functionality:Restore Priority:2 Issues for the current backlog. Style:PackageReference Type:Feature
Projects
None yet
Development

No branches or pull requests

8 participants