fix pack targets to add tfm metadata to output lib files #1255

rohit21agrawal · 2017-03-21T23:32:53Z

Fixes: NuGet/Home#4840
Fixes: NuGet/Home#4698
Fixes: NuGet/Home#4704

This is long overdue work of adding TargetFramework metadata to build items that need to go into the lib folder in the nupkg. By adding this metadata, we know precisely where in the lib folder an item will go instead of having to search for framework in the path of the file on disk.
This change also adds outputs from SatelliteDLLsProjectOutputGroupOutput and does the appropriate plumbing so they go under their respective culture folders.
An extension point has been provided for executing custom target in the inner build for files that go in the lib (or folder specified via BuildOutputTargetFolder) . A user can write their own custom target and specify it as the value of the property $(TargetsForTfmSpecificBuildOutput). The target should write out any files that need to go into the lib folder into the ItemGroup BuildOutputInPackage and set the following two metadata values :
a) FinalOutputPath : This is the absolute path of the file on disk (if not provided, the Identity is used to evaluate source path)
b) TargetPath : This is optional and defaults to the name of the file. Users need to set it when the file needs to go into a subfolder within lib\<TFM> like satellite assemblies which go under their respective culture folders.
An extension point has been provided for files that need to go in the nupkg outside of lib folder but are still TFM specific. A user can write their own custom target and specify it as the value of the property $(TargetsForTfmSpecificContentInPackage). The target should write out any files that need to be included in the nupkg into the ItemGroup TfmSpecificPackageFile and set the following metadata:
a) PackagePath : Path where the file should be output in the package. NuGet will throw a warning if more than one file is added to the same package path.
b) BuildAction: Default value has been set to None. This is only required if the package path is in contentFiles folder.

CC: @emgarten @alpaix @jainaashish @nkolev92 @rrelyea @mishra14 @zhili1208

CC: @AArnott to take a look if i covered most of his feedback in NuGet/Home#4698

CC: @onovotny for point 3 and point 4. Would appreciate any feedback on the design.

emgarten · 2017-03-21T23:58:19Z

src/NuGet.Core/NuGet.Build.Tasks.Pack.Library/PackTaskLogic.cs

+                    targetPath = Path.GetFileName(finalOutputPath);
+                }
+
+                if (string.IsNullOrEmpty(targetFramework) || NuGetFramework.Parse(targetFramework) == null)


I think parse always returns a framework, you may want to check NuGetFramework.Parse(targetFramework).IsSpecificFramework == false which would mean it came back as unsupported or something else that a project should not have.

@emgarten fixed.

emgarten · 2017-03-22T00:04:42Z

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void PackCommand_PackConsoleAppWithRID_NupkgValid(bool includeSymbols)


Are there tests for satellite assemblies?

emgarten · 2017-03-22T00:09:06Z

src/NuGet.Core/NuGet.Build.Tasks.Pack.Library/Pack.targets

+          Returns="@(_TargetPathsToAssembliesWithTfm)">
+    <ItemGroup>
+      <_TargetPathsToAssembliesWithTfm Include="@(SatelliteDllsProjectOutputGroupOutput)">
+        <TargetFramework>$(TargetFramework)</TargetFramework>


Should these have a type property? Any chance they could get mixed up later?

@emgarten as discussed offline, there is no need for type , as the TargetPath metadata automatically adds culture to the filename.

emgarten · 2017-03-22T00:10:02Z

src/NuGet.Core/NuGet.Build.Tasks.Pack.Library/PackTaskLogic.cs

+
+                if (!File.Exists(finalOutputPath))
+                {
+                    throw new FileNotFoundException(string.Format(Strings.Error_FileNotFound, finalOutputPath));


nit: set culture to current culture with string.Format, not needed with one param, but generally good to do for user facing errors.

emgarten

🚀

clairernovotny · 2017-03-23T01:28:03Z

How would 4 look in practice?

I imagine that for things like .pp transforms where I'd want them to be included, I could use the existing <None Include="Foo.xml.cs.pp" /> and extend it with Metadata -- BuildAction and PackagePath and it'd "just work."

Not sure why I'd need another set of targets? Having them as None in the solution explorer lets them show up in my solution easily.

rohit21agrawal · 2017-03-23T01:33:44Z

@onovotny yes, for anything that is not TFM specific, that would work. But once you have any content (or even build output) that is TFM specific and not covered by the default pack targets, 3 and 4 give you the flexibility to put it at the desirable location within the nuget package.

While (4) is generic enough to cover the functionality of (3), (3) provides stricter control about files that need to go into the lib folder, like setting the right TFM folder names.

emgarten

💯

kzu · 2017-04-11T15:20:07Z

again diverges significantly from the simple approach taken by NuGetizer: https://github.com/NuGet/NuGet.Build.Packaging/blob/dev/src/Build/NuGet.Build.Packaging.Tests/AssignPackagePathTests.cs#L273-L275

In Nugetizer, you simply annotate items with the kind (i.e. Lib, Build, Tools) and the right thing happens. Lib, puts the item in the TFM-specific folder according to the project's TF.

If you know where you want to put it instead, you just add either TFM or a TF to the item. That's it. No additional targets, no additional properties.

rohit21agrawal · 2017-04-11T17:06:58Z

@kzu 4) is just an extension point for power users and not the default way to pack things, which we implemented because users expressed the need for it. Content is usually not TFM-specific, and that is why default targets include content only in the outer build , however, 4) provides a way for the unusual scenario of including tfm-specific content.

how do you determine which items to check for the pack-specific metadata?

mhutch · 2017-04-12T22:23:30Z

@rohit21agrawal it's documented in the detail in the NuGetizer spec: https://github.com/NuGet/Home/wiki/NuGetizer-Core-Features

AArnott · 2017-05-05T01:24:55Z

Which milestone of VS will this ship with, please?

rohit21agrawal · 2017-05-05T01:25:30Z

15.3

dnfclas added the cla-already-signed label Mar 21, 2017

emgarten reviewed Mar 21, 2017

View reviewed changes

emgarten reviewed Mar 22, 2017

View reviewed changes

rohit21agrawal force-pushed the dev-ragrawal-packoutputfix branch 2 times, most recently from 810cfdb to ad296ee Compare March 22, 2017 00:17

emgarten approved these changes Mar 22, 2017

View reviewed changes

rohit21agrawal force-pushed the dev-ragrawal-packoutputfix branch 5 times, most recently from bb44dc0 to b8123cc Compare March 22, 2017 22:17

fix pack targets to add tfm metadata to output lib files

98c6ba1

rohit21agrawal force-pushed the dev-ragrawal-packoutputfix branch from b8123cc to 98c6ba1 Compare March 22, 2017 23:42

emgarten approved these changes Mar 23, 2017

View reviewed changes

rohit21agrawal merged commit 43afe12 into dev Mar 23, 2017

natemcmaster mentioned this pull request Mar 23, 2017

Need ability to pack files from build output folder in multi-targeting projects NuGet/Home#4704

Closed

rohit21agrawal deleted the dev-ragrawal-packoutputfix branch March 23, 2017 18:13

rohit21agrawal mentioned this pull request Mar 24, 2017

msbuild /t:pack does not build with Entity Framework csdl/msl/ssdl model files NuGet/Home#4777

Closed

JeremySkinner mentioned this pull request Apr 7, 2017

Nuget package without language resources FluentValidation/FluentValidation#386

Closed

rohit21agrawal mentioned this pull request Apr 14, 2017

Producing an MSBuild task package dotnet/sdk#1125

Closed

livarcocc mentioned this pull request Apr 19, 2017

Producing an MSBuild task package NuGet/Home#5063

Open

rohit21agrawal mentioned this pull request May 4, 2017

AppendTargetFrameworkToOutputPath=false causes packaging errors for multi-targeting projects NuGet/Home#5160

Closed

bording mentioned this pull request Jul 18, 2017

Use official extension point to add PDBs Particular/NServiceBus#4857

Merged

ctaggart mentioned this pull request Aug 6, 2017

Update suggestion for how to include PDBs in package ctaggart/SourceLink#231

Closed

rohit21agrawal mentioned this pull request Aug 15, 2017

Adding assemblies to package with csproj messes up the build output NuGet/Home#5523

Closed

natemcmaster mentioned this pull request Dec 7, 2017

Add Internal.AspNetCore.SiteExtension.Sdk package aspnet/BuildTools#505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix pack targets to add tfm metadata to output lib files #1255

fix pack targets to add tfm metadata to output lib files #1255

rohit21agrawal commented Mar 21, 2017 •

edited

Loading

emgarten Mar 21, 2017

rohit21agrawal Mar 22, 2017

emgarten Mar 22, 2017

emgarten Mar 22, 2017

rohit21agrawal Mar 22, 2017

emgarten Mar 22, 2017

emgarten left a comment

clairernovotny commented Mar 23, 2017

rohit21agrawal commented Mar 23, 2017 •

edited

Loading

emgarten left a comment

kzu commented Apr 11, 2017 •

edited

Loading

rohit21agrawal commented Apr 11, 2017 •

edited

Loading

mhutch commented Apr 12, 2017

AArnott commented May 5, 2017

rohit21agrawal commented May 5, 2017

fix pack targets to add tfm metadata to output lib files #1255

fix pack targets to add tfm metadata to output lib files #1255

Conversation

rohit21agrawal commented Mar 21, 2017 • edited Loading

emgarten Mar 21, 2017

Choose a reason for hiding this comment

rohit21agrawal Mar 22, 2017

Choose a reason for hiding this comment

emgarten Mar 22, 2017

Choose a reason for hiding this comment

emgarten Mar 22, 2017

Choose a reason for hiding this comment

rohit21agrawal Mar 22, 2017

Choose a reason for hiding this comment

emgarten Mar 22, 2017

Choose a reason for hiding this comment

emgarten left a comment

Choose a reason for hiding this comment

clairernovotny commented Mar 23, 2017

rohit21agrawal commented Mar 23, 2017 • edited Loading

emgarten left a comment

Choose a reason for hiding this comment

kzu commented Apr 11, 2017 • edited Loading

rohit21agrawal commented Apr 11, 2017 • edited Loading

mhutch commented Apr 12, 2017

AArnott commented May 5, 2017

rohit21agrawal commented May 5, 2017

rohit21agrawal commented Mar 21, 2017 •

edited

Loading

rohit21agrawal commented Mar 23, 2017 •

edited

Loading

kzu commented Apr 11, 2017 •

edited

Loading

rohit21agrawal commented Apr 11, 2017 •

edited

Loading