Dotnet publish with filter profile #685

ramarag · 2017-01-19T01:08:43Z

@schellap @eerhardt @dsplaisted @nguerrera @gkhanna79 @srivatsn PTAL
Implements #555

nil4 · 2017-01-19T10:11:26Z

src/Tasks/Microsoft.NET.Build.Tasks/LockFileExtensions.cs

@@ -76,6 +83,38 @@ public static LockFileTargetLibrary GetLibrary(this LockFileTarget lockFileTarge

            return projectDeps;
        }
+
+        public static HashSet<string> GetIntersetction(


Typo: Intersetction

nil4 · 2017-01-19T10:12:05Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Publish.targets

+  <Target Name="ComputeAssembliesToBeFiltered"
+          Condition="'$(FilterProjFile)'!=''" >
+    <PropertyGroup>
+      <FilterProFileDir>$(MSBuildProjectExtensionsPath)\filterprofile</FilterProFileDir>


Typo: FilterProFile (j missing?)

yup, corrected

eerhardt · 2017-01-19T16:58:34Z

src/Tasks/Microsoft.NET.Build.Tasks/LockFileExtensions.cs

@@ -76,6 +83,38 @@ public static LockFileTargetLibrary GetLibrary(this LockFileTarget lockFileTarge

            return projectDeps;
        }
+
+        public static HashSet<string> GetIntersetction(
+            IDictionary<string, LockFileTargetLibrary> coll1,


It is standard coding in C# to not use abbreviations or contractions in variable names.

coll1
iter

eerhardt · 2017-01-19T17:00:11Z

src/Tasks/Microsoft.NET.Build.Tasks/LockFileExtensions.cs

@@ -76,6 +83,38 @@ public static LockFileTargetLibrary GetLibrary(this LockFileTarget lockFileTarge

            return projectDeps;
        }
+
+        public static HashSet<string> GetIntersetction(


This method doesn't really seem to belong in LockFileExtensions, since it is not an extension method.

My thought would be to make it a private method next to where it is called, and if it needs to be shared somewhere, it can be moved to an appropriate place later.

eerhardt · 2017-01-19T17:02:04Z

src/Tasks/Microsoft.NET.Build.Tasks/ProjectContext.cs

+            {
+                IEnumerable<LockFileTargetLibrary> filterLibraries = _filterlockFileTarget.Libraries;
+                Dictionary<string, LockFileTargetLibrary> filterLookup =
+                filterLibraries.ToDictionary(e => e.Name, StringComparer.OrdinalIgnoreCase);


(nit) whitespace here. This line should be indented since it is a continuation of the line above it.

eerhardt · 2017-01-19T17:04:53Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePublishAssemblies.cs

@@ -45,12 +47,20 @@ protected override void ExecuteCore()
            IEnumerable<string> privateAssetsPackageIds = PackageReferenceConverter.GetPackageIds(PrivateAssetsPackageReferences);
            IPackageResolver packageResolver = NuGetPackageResolver.CreateResolver(lockFile, ProjectPath);

+            LockFile FilterLockFile = null;


(nit) local variables should be camelCased.

eerhardt · 2017-01-19T17:05:31Z

src/Tasks/Microsoft.NET.Build.Tasks/ResolvePublishAssemblies.cs

+            LockFile FilterLockFile = null;
+            if (!string.IsNullOrEmpty(FilterProjectAssetsFile))
+            {
+                FilterLockFile = new LockFileCache(BuildEngine4).GetLockFile(FilterProjectAssetsFile);


Can reuse the LockFileCache created above, so we don't need to create 2 instances of this object in the same method.

Correct me if i am missing something, i thought LockFileCache maps to a single asset file. We are dealing with two asset files here
AssetsFilePath - the App project
FilterLockFile - the Filter profile supplied

You are right that we are using 2 assets files. But what I'm saying is there is no point in creating two instances of the LockFileCache object. The same cache object can load multiple asset files.

ah got it , you did not want to new up another object

eerhardt · 2017-01-19T17:08:54Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Publish.targets

+          Condition="'$(FilterProjFile)'!=''" >
+    <PropertyGroup>
+      <FilterProFileDir>$(MSBuildProjectExtensionsPath)\filterprofile</FilterProFileDir>
+      <FilterProjectAssetsFile>$(FilterProFileDir)\project.assets.json</FilterProjectAssetsFile>


Where does this file get created?

This is created in https://github.com/dotnet/sdk/pull/685/files#diff-ffd1a974928a37a20fbc471c040962acR265 as product of restore on the filterprojfile

ramarag · 2017-01-24T02:22:43Z

@eerhardt i have addressed your comments

eerhardt · 2017-01-24T17:09:22Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Publish.targets

+
+ <!--
+    ============================================================
+                     RunResolvePublishAssemblies


This comment and summary are outdated.

Oops will correct it

eerhardt · 2017-01-24T17:12:14Z

src/Tasks/Microsoft.NET.Build.Tasks/RuntimeOptions.cs

@@ -9,6 +9,8 @@ namespace Microsoft.NET.Build.Tasks
 {
    internal class RuntimeOptions
    {
+        public string tfm { get; set; }


The property doesn't really align with the rest of the properties in the .json file.

Its casing is different than the existing properties.

It is an abbreviation, where the rest aren't.

Runtime options are only read by the dotnet host, so we have some leeway here

eerhardt · 2017-01-24T17:14:11Z

src/Tasks/Microsoft.NET.Build.Tasks/RuntimeOptions.cs

@@ -9,6 +9,8 @@ namespace Microsoft.NET.Build.Tasks
 {
    internal class RuntimeOptions
    {
+        public string tfm { get; set; }


Why can't this value be inferred from the existing "Framework" property? Also, TFMs don't have patch numbers in them. So if we had a bug fix in crossgen between 1.1.0 and 1.1.1, how would the cache get updated? The assembly was crossgened using 1.1.0 and cached in "netcoreapp1.1", but when I installed 1.1.1, there is no way to get the cache updated, because the assembly was already cached for "netcoreapp1.1".

The caching step is typically done by admin or installer scripts. We do not expect the end user to do this themselves.

When this feature was spec'd, we decided to not do side by side install for the same TFM, to fix the crossgen bug- we would replace the assemblies already cached with the updated assembles, using the above mechanism

The readytorun images are supposed to be resilient to runtime changes within the same TFM, worst case we are to fallback to jitting

@gkhanna79 : Do correct me if i am wrong

@ramarag is correct.

eerhardt · 2017-01-24T17:19:02Z

src/Tasks/Microsoft.NET.Build.Tasks/GenerateRuntimeConfigurationFiles.cs

@@ -28,6 +28,9 @@ public class GenerateRuntimeConfigurationFiles : TaskBase
        public string TargetFramework { get; set; }

        [Required]
+        public string TargetFrameworkMoniker { get; set; }


Taking both TargetFramework and TargetFrameworkMoniker seems redundant.

i had a chat with @nguerrera there seems to be no other clean way

There seems to be no good way to get reliably the same TargetFramework which was used to create the TargetFrameworkMoniker. Since we use the user supplied TargetFramework at build time, we want it to be the same here

…list of packages

…0190604.11 (dotnet#685) - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.0.0-preview6.19304.11 - Microsoft.AspNetCore.Mvc.Analyzers - 3.0.0-preview6.19304.11

dnfclas added the cla-already-signed label Jan 19, 2017

ramarag force-pushed the dotnet-filter branch from aa7d295 to 7d6e836 Compare January 19, 2017 01:41

ramarag self-assigned this Jan 19, 2017

nil4 reviewed Jan 19, 2017

View reviewed changes

eerhardt reviewed Jan 19, 2017

View reviewed changes

ramarag force-pushed the dotnet-filter branch from 7d6e836 to 243e12f Compare January 24, 2017 02:21

ramarag force-pushed the dotnet-filter branch from 243e12f to db24f59 Compare January 24, 2017 03:11

eerhardt reviewed Jan 24, 2017

View reviewed changes

ramarag added 2 commits January 24, 2017 11:43

Adding a FilterProfile option to filter out the closure of specified …

a07294d

…list of packages

Integration test for publish filter profile

c11f9f7

ramarag force-pushed the dotnet-filter branch from db24f59 to c11f9f7 Compare January 24, 2017 19:44

ramarag merged commit 2d2957d into dotnet:future Jan 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dotnet publish with filter profile #685

Dotnet publish with filter profile #685

ramarag commented Jan 19, 2017 •

edited

Loading

nil4 Jan 19, 2017

ramarag Jan 23, 2017

nil4 Jan 19, 2017

ramarag Jan 23, 2017

eerhardt Jan 19, 2017

ramarag Jan 23, 2017

eerhardt Jan 19, 2017

ramarag Jan 23, 2017

eerhardt Jan 19, 2017

ramarag Jan 23, 2017

eerhardt Jan 19, 2017

eerhardt Jan 19, 2017

ramarag Jan 19, 2017

eerhardt Jan 19, 2017

ramarag Jan 23, 2017

eerhardt Jan 19, 2017

ramarag Jan 19, 2017

ramarag commented Jan 24, 2017

eerhardt Jan 24, 2017

ramarag Jan 24, 2017

eerhardt Jan 24, 2017

ramarag Jan 24, 2017

eerhardt Jan 24, 2017

ramarag Jan 24, 2017

gkhanna79 Jan 24, 2017

eerhardt Jan 24, 2017

ramarag Jan 24, 2017

ramarag Jan 24, 2017

Dotnet publish with filter profile #685

Dotnet publish with filter profile #685

Conversation

ramarag commented Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramarag commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramarag commented Jan 19, 2017 •

edited

Loading