Dotnet cache #591

ramarag · 2017-01-05T22:23:13Z

@schellap @nguerrera @eerhardt @gkhanna79 @srivatsn PTAL

This is the implementation towards #554

ramarag · 2017-01-05T22:30:23Z

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

+            if (pkginfo != null)
+            {
+                packageDir  = pkginfo.PathResolver.GetInstallPath(packageId, version);
+                packageRoot = pkginfo.PathResolver.GetVersionListPath("");  //TODO Check with nuget  if this is the correct way to get the package root


This is a hack for now, i filed an issue with nuget client and got them to get a change- NuGet/NuGet.Client#1096, that we will use as
FallbackPackagePathInfo.PathResolver.RootPath to get the packageRoot.

If this isn't fixed by the time you check this in, it would be a good idea to put this info into a comment until we can take that new nuget change.

In reply to: 94865939 [](ancestors = 94865939)

Please file an issue on yourself to clean this up and reference the issue in the code. NIT: spacing issues

ramarag · 2017-01-06T00:05:33Z

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

+    Cleans up the temp directory used by optimize phase
+    ============================================================
+    -->
+  <Target Name="_CleanupTempFiles"


will also do this at the start of cache phase

ramarag · 2017-01-06T00:06:13Z

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

+      <PublishDir Condition="'$(ComposeDir)' == '' and '$(RuntimeIdentifier)' != ''">$(OutputPath)$(RuntimeIdentifier)\</PublishDir>
+
+      <FX_Version Condition="'$(FX_Version)' == ''">$(_ShortFrameworkVersion)</FX_Version>
+      <_CrossProjFileDir>$([System.IO.Path]::Combine($([System.IO.Path]::GetTempPath()),"Optimize_Crossgen_$(RuntimeIdentifier)_$(TargetFramework)_$(FX_Version)"))</_CrossProjFileDir>


should change this to be unique to every invocation

dsplaisted

Did you mean to include .Microsoft.NET.ComposeCache.targets.swp? It's showing up as a binary file and I don't know what it's for.

dsplaisted · 2017-01-05T23:06:20Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/Mocks/MockPackageResolver.cs

+        public string GetPackageDirectory(string packageId, NuGetVersion version, out string packageRoot)
+        {
+            packageRoot = _root;
+            return Path.Combine(_root, packageId, version.ToNormalizedString(), "path");


Call the previous overload of this function here instead of duplicating the logic.

dsplaisted · 2017-01-05T23:09:08Z

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

@@ -24,27 +25,38 @@ public PublishAssembliesResolver WithPrivateAssets(IEnumerable<string> privateAs
            _privateAssetPackageIds = privateAssetPackageIds;
            return this;
        }
+        public PublishAssembliesResolver PreserveCacheLayout(string cond)


Why is this method taking a string and trying to convert to a bool instead of just taking a bool directly? Also, the parameter should be named preserveCacheLayout or similar instead of cond.

NIT: With PreserveCacheLayout().

dsplaisted · 2017-01-05T23:51:46Z

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

@@ -29,7 +29,8 @@ public class ResolvePublishAssemblies : TaskBase
        public string PlatformLibraryName { get; set; }

        public ITaskItem[] PrivateAssetsPackageReferences { get; set; }
-
+
+        public string PreserveCacheLayout { get; set; }


Change this to be of type bool and MSBuild will handle the conversion.

dsplaisted · 2017-01-06T00:24:01Z

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

+      <PublishDir Condition="'$(ComposeDir)' == '' and '$(RuntimeIdentifier)' != ''">$(OutputPath)$(RuntimeIdentifier)\</PublishDir>
+
+      <FX_Version Condition="'$(FX_Version)' == ''">$(_ShortFrameworkVersion)</FX_Version>
+      <_CrossProjFileDir>$([System.IO.Path]::Combine($([System.IO.Path]::GetTempPath()),"Optimize_Crossgen_$(RuntimeIdentifier)_$(TargetFramework)_$(FX_Version)"))</_CrossProjFileDir>


I think using a path inside the temp path for this risks build failures if two different builds on the same machine try to write to the same folder.

Is there any reason not to use a folder under the intermediate output path for this?

i have already called this out in #591 (comment)

dsplaisted · 2017-01-06T00:27:49Z

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

+    ============================================================
+    -->
+  <Target Name="_CopyResolvedManagedFiles"
+          DependsOnTargets="_ComputeResolvedFilesToCachTypes;


It looks like there's an 'e' missing in "Cache" for _ComputeResolvedFilesToCachTypes

dsplaisted · 2017-01-06T00:31:53Z

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

+      <_AssembliestoCrossGen Include="$(_RuntimeDir)\**\*.*" />
+    </ItemGroup>
+
+    <Error Text="Error more than one coreclr found !" Condition="'@(_CoreclrResolvedPath->Count())' &gt; 1" />


Strings like these are supposed to be added to Strings.resx and then generated during the build with the NETSdkError task, so they can be localized.

dsplaisted · 2017-01-06T00:35:30Z

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

+      <_CoreclrDir>$([System.IO.Path]::GetDirectoryName($(_CoreclrPath)))</_CoreclrDir>
+      <_CoreclrDir>$([System.IO.Path]::GetDirectoryName($(_CoreclrDir)))</_CoreclrDir>
+      <_CoreclrDir>$([System.IO.Path]::GetDirectoryName($(_CoreclrDir)))</_CoreclrDir>
+      <_CoreclrDir>$([System.IO.Path]::GetDirectoryName($(_CoreclrDir)))</_CoreclrDir>


It looks like you're trying to get the relative path ..\..\..\tools from where the CoreCLR was resolved. I think you can probably do this much more cleanly with Path.GetFullPath() or a similar method.

dsplaisted · 2017-01-06T00:36:52Z

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

+      <_CrossgenDir>$([System.IO.Path]::Combine($(_CoreclrDir),"tools"))</_CrossgenDir>
+      <!-- TODO override with rid specific tools path for x-arch -->
+      <_Crossgen>$([System.IO.Path]::Combine($(_CrossgenDir),"crossgen"))</_Crossgen>
+      <_Crossgen Condition="$(RuntimeIdentifier.StartsWith('win'))">$([System.IO.Path]::Combine($(_CrossgenDir),"crossgen.exe"))</_Crossgen>


It seems like this should be based on the current OS you're running on, rather than the target you're crossgen'ing for.

dsplaisted · 2017-01-06T00:39:43Z

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

+      <RuntimeIdentifier>$(RuntimeIdentifier)</RuntimeIdentifier>
+  </PropertyGroup>
+  <ItemGroup>
+       <PackageReference Include="Microsoft.NETCore.App" Version="$(FX_Version)" />


I don't think you need this any more as of #450

Actually the Version of NetcoreApp is specified by the user

eerhardt · 2017-01-06T22:04:04Z

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

@@ -53,13 +54,14 @@ protected override void ExecuteCore()
            IEnumerable<ResolvedFile> resolvedAssemblies = 
                new PublishAssembliesResolver(packageResolver)
                    .WithPrivateAssets(privateAssetsPackageIds)
+                    .PreserveCacheLayout(PreserveCacheLayout)


This method should follow the same naming as the existing method. WithPreserveCacheLayout.

eerhardt · 2017-01-06T22:06:10Z

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

@@ -13,6 +13,7 @@ public class PublishAssembliesResolver
    {
        private readonly IPackageResolver _packageResolver;
        private IEnumerable<string> _privateAssetPackageIds;
+        private bool _preserveCacheLayout;


Can you add a unit test or two for the new behavior?

Along with that idea - it would be good to add an integration test or two in https://github.com/dotnet/sdk/blob/master/test/Microsoft.NET.Publish.Tests/.

👍 for @eerhardt's comment. @ramarag it is important that an end to end test exercises this functionality as dotnet cache is the only one that'd exercise the preserveCacheLayout. Please file an issue and try to close it when you are ready to commit this PR.

Added tests

eerhardt · 2017-01-06T22:07:44Z

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


                foreach (LockFileRuntimeTarget runtimeTarget in targetLibrary.RuntimeTargets.FilterPlaceHolderFiles())
                {
                    if (string.Equals(runtimeTarget.AssetType, "native", StringComparison.OrdinalIgnoreCase) ||
                        string.Equals(runtimeTarget.AssetType, "runtime", StringComparison.OrdinalIgnoreCase))
                    {
+                        string srcpath = Path.Combine(libraryPath, runtimeTarget.Path);


(nit) the casing/naming of this variable doesn't really match existing code. It could be srcPath or sourcePath.

eerhardt · 2017-01-06T22:11:03Z

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

+        {
+            if (_preserveCacheLayout)
+            {
+                destpath = Path.GetDirectoryName(libraryPath.Replace(pkgRoot, ""));


Do you want to replace all the occurences? Or just the beginning? Maybe checking if the libraryPath starts with pkgRoot, then substring the pkgRoot length off of libraryPath.

eerhardt · 2017-01-06T22:11:53Z

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

        {
            SourcePath = Path.GetFullPath(sourcePath);
            DestinationSubDirectory = destinationSubDirectory;
+            AssetType = String.IsNullOrEmpty(assetType) ? String.Empty : assetType.ToLower();


(nit) the typical coding in the dotnet repos is to use string not String.

Why does assetType get a default null?

eerhardt · 2017-01-06T22:13:32Z

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

+    ============================================================
+                                        _RunOptimizer
+
+    Start the optimization face


face? Did you mean phase?

yup, thanks

eerhardt · 2017-01-06T22:13:55Z

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

+    ============================================================
+                                        ComposeCache
+
+    The main publish entry point.


this must be a copy-paste error.

eerhardt · 2017-01-06T22:14:23Z

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

+  <!--
+    ============================================================
+                                        _RestoreCrossgen
+Restores netcoreapp and pulbishes it to a temp directory


typeo pulbishes

eerhardt · 2017-01-06T22:14:52Z

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

+    ============================================================
+    -->
+  <Target Name="_RestoreCrossgen">
+    <!-- Restore Crossgen-->


(nit) this comment doesn't really add value since it is just repeating the name of the target.

eerhardt · 2017-01-06T22:20:44Z

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

+    ============================================================
+    -->
+  <Target Name="_RunOptimizer"
+          DependsOnTargets="_ComputeResolvedFilesToCachTypes;


(nit) Cach

eerhardt · 2017-01-06T22:23:19Z

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

+      <!-- TODO override with rid specific tools path for x-arch -->
+      <_Crossgen>$([System.IO.Path]::Combine($(_CrossgenDir),"crossgen"))</_Crossgen>
+      <_Crossgen Condition="$(RuntimeIdentifier.StartsWith('win'))">$([System.IO.Path]::Combine($(_CrossgenDir),"crossgen.exe"))</_Crossgen>
+      <_JitPath>$([System.IO.Path]::Combine($(_NetCoreRefDir),"libclrjit.so"))</_JitPath>


Does this support OSX? Isn't it dylib on OSX?

this should be fixed now

eerhardt · 2017-01-06T22:24:53Z

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

+                                             Condition="'%(CrossgenResolvedAssembliesToPublish.Filename)'=='coreclr'" />
+      <_CoreclrResolvedPath Include="@(CrossgenResolvedAssembliesToPublish)"
+                                             Condition="'%(CrossgenResolvedAssembliesToPublish.Filename)'=='libcoreclr'" />
+      <_AssembliestoCrossGen Include="$(_RuntimeDir)\**\*.*" />


Don't we want to crossgen ALL of the app's references and the app itself?

Only the managed assemblies, which are copied in step https://github.com/dotnet/sdk/pull/591/files/f8e0bd7d906be1ae3d1f8b105bf539455c3da6ce#diff-8a3d011306d4fb49ae22e3b9f51e2811R309

gkhanna79 · 2017-01-10T19:19:20Z

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

@@ -8,5 +8,6 @@ namespace Microsoft.NET.Build.Tasks
    public interface IPackageResolver
    {
        string GetPackageDirectory(string packageId, NuGetVersion version);
+        string GetPackageDirectory(string packageId, NuGetVersion version, out string packageRoot);


We shouldn't need this per the earlier comment.

the earlier comment #591 (comment) is referring to duplication of logic within the function and not its usage

@srivatsn, what's going to be our policy on breaking changes to API surface area in Microsoft.NET.Build.Tasks. We have stuff that's public for unit testability but not intended to be called by customers. Should we lock it down with IVTA before RTM?

This one is only breaking if someone implements the interface, which I doubt anyone would do, but my experience with these sorts of things on corefx was traumatic.

@nguerrera correct me if i am wrong- IPackageResolver is more like the interface sdk consumes, kind of like PAL that abstracts Nuget package management. So if the interface changes it is unlikely to ever break our consumers.

As of now this interface is only implemented by sdk

Nothing in Microsoft.NET.Build.Tasks.dll is meant to be consumed by callers outside of the .targets files in the dotnet/sdk repo. Yes, it says public and people could try to use the types in this assembly, but we don't package them in any consumable way - i.e. no NuGet package that you can reference and now refer to types in the assembly. If someone tries, and they get broken in the future, they need to fix themselves.

The reason we can't make these types 'internal' and use IVTA right now is because #55 isn't fixed. So the signing of the assemblies gets messed up cross-platform, which leads to complications with IVTA.

If someone tries, and they get broken in the future, they need to fix themselves.

My experience with these statements is that we make them while building v1 and then get blocked from actually breaking anything in v2 no matter how bizarre the use case. I'd rather make things explicitly internal to just avoid having to even discuss such a policy.

The reason we can't make these types 'internal' and use IVTA right now is because #55 isn't fixed.

The current skip-signing condition is too aggressive. Public signing works fine on non-Windows and would allow us to use IVTA with the same key across all platforms.

@ramarag @gkhanna79 You can disregard this for this PR.

gkhanna79 · 2017-01-10T19:21:02Z

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

+                packageDir  = pkginfo.PathResolver.GetInstallPath(packageId, version);
+                packageRoot = pkginfo.PathResolver.GetVersionListPath("");  //TODO Check with nuget  if this is the correct way to get the package root
+            }
+            return packageDir;


Shouldnt the invariant of this function be that it will always return non-null value?

gkhanna79 · 2017-01-10T19:23:18Z

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


-                results.AddRange(GetResolvedFiles(targetLibrary.RuntimeAssemblies, libraryPath));
-                results.AddRange(GetResolvedFiles(targetLibrary.NativeLibraries, libraryPath));
+                results.AddRange(GetResolvedFiles(targetLibrary.RuntimeAssemblies, libraryPath, pkgRoot, "runtime"));


Will being more specific (about "runtime" and "native") affect other scenarios that use this Resolve method?

I believe targetLibrary.RuntimeAssemblies by definition are "runtime" assemblies

I am trying to understand what difference your change brings in from what the code currently is. Can you please elaborate?

When results are getting populated previously we used to lose the information on the type of assemblies, [ their type was explicit due to their nuget layout and way nuget provides them as different lists like-
RuntimeAssemblies - managed code
NativeLibraries - unmanaged code

I am just adding back this information, to be used in filtering the list by the msbuild targets

gkhanna79 · 2017-01-10T19:26:46Z

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

-                                sourcePath: Path.Combine(libraryPath, runtimeTarget.Path),
-                                destinationSubDirectory: GetRuntimeTargetDestinationSubDirectory(runtimeTarget)));
+                                sourcePath: srcpath,
+                                destinationSubDirectory: GetDestinationSubDirectory(srcpath,


Wouldnt this result in nullRef exception when GetDestinationSubDirectory returns null?

GetDestinationSubDirectory is not going to return null, unless the path needed for preservecachelayout=false is null ( i.e GetRuntimeTargetDestinationSubDirectory(runtimeTarget)

gkhanna79 · 2017-01-10T19:27:34Z

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

 namespace Microsoft.NET.Build.Tasks
 {
    public class ResolvedFile
    {
        public string SourcePath { get; }

        public string DestinationSubDirectory { get; }
-
+        public string AssetType { get; }


Why not an enum instead of string?

ramarag · 2017-01-14T04:57:48Z

@dsplaisted @eerhardt @schellap can i get a sign off ?

@srivatsn I do not have any write privileges on the sdk repo, can you do the needful ?

gkhanna79 · 2017-01-14T05:19:04Z

Can this method not be in a second interface so as to not introduce a breaking change? Thanks, Gaurav

-------- Original message -------- From: Nick Guerrera <notifications@github.com> Date: 1/13/17 9:03 PM (GMT-08:00) To: dotnet/sdk <sdk@noreply.github.com> Cc: "Gaurav Khanna (CLR)" <Gaurav.Khanna@microsoft.com>, Mention <mention@noreply.github.com> Subject: Re: [dotnet/sdk] Dotnet cache (#591) @nguerrera commented on this pull request.

________________________________ In src/Tasks/Microsoft.NET.Build.Tasks/IPackageResolver.cs<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F591&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7Cd4195c84319f40af912f08d43c3ab096%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636199670149292929&sdata=W7K%2F%2FRTLsLyE6NIWap%2FeGxmJyKd8ezE1J6Dk722xK9I%3D&reserved=0>:

@@ -8,5 +8,6 @@ namespace Microsoft.NET.Build.Tasks

public interface IPackageResolver { string GetPackageDirectory(string packageId, NuGetVersion version); + string GetPackageDirectory(string packageId, NuGetVersion version, out string packageRoot); This one is only breaking if someone implements the interface, which I doubt anyone would do, but my experience with these sorts of things on corefx was traumatic. - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fsdk%2Fpull%2F591&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7Cd4195c84319f40af912f08d43c3ab096%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636199670149292929&sdata=W7K%2F%2FRTLsLyE6NIWap%2FeGxmJyKd8ezE1J6Dk722xK9I%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJA3wWNP1Vuu5scu2ZZvZiBt5DHNqerZks5rSFcjgaJpZM4LcMkm&data=02%7C01%7CGaurav.Khanna%40microsoft.com%7Cd4195c84319f40af912f08d43c3ab096%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636199670149292929&sdata=tCiNVh8WVGlnqCsGeSpQNMkx5%2FMFjQ9p9EYBFjQEcPA%3D&reserved=0>.

ramarag · 2017-01-17T23:31:49Z

I have made all changes that are required for dotnet cache in sdk repo
@dsplaisted @eerhardt @schellap @nguerrera is there anything else that needs to be changed ?

signoffs ?

eerhardt · 2017-01-18T15:20:35Z

TestAssets/TestProjects/SimpleCache/SimpleCache.csproj

+<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0">
+    <ItemGroup>
+        <PackageReference Include="System.Private.Uri" Version="4.4.0-beta-24821-02" />
+        <PackageReference Include="Microsoft.NETCore.CoreDisTools" Version="1.0.1-prerelease-00001" />


Does this package need to be referenced in order to run the ComposeCache target? Should there be a test of a "normal" test app, like https://github.com/dotnet/sdk/blob/master/TestAssets/TestProjects/SimpleDependencies/SimpleDependencies.csproj?

The cache step is used to prime the assemblies. the run step will only occur after publish with filter profile. The CoreDistTools are unmanaged native component. I do not see the need for a SimpleManaged test. Will add one when i do publish PR

eerhardt · 2017-01-18T15:24:11Z

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

 namespace Microsoft.NET.Build.Tasks
 {
+    public enum AssetType
+    {
+        none,


Enum values are normally PascalCased.

DayOfWeek.Sunday, StringComparison.Ordinal, etc.

eerhardt · 2017-01-18T15:24:27Z

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

                other.SourcePath == SourcePath &&
                other.DestinationSubDirectory == DestinationSubDirectory;
+               ;


unnecessary

eerhardt · 2017-01-18T15:24:58Z

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

@@ -29,7 +29,8 @@ public class ResolvePublishAssemblies : TaskBase
        public string PlatformLibraryName { get; set; }

        public ITaskItem[] PrivateAssetsPackageReferences { get; set; }
-
+
+        public bool preserveCacheLayout { get; set; }


Public properties should be PascalCased.

eerhardt · 2017-01-18T15:27:50Z

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

 namespace Microsoft.NET.Build.Tasks
 {
+    public enum AssetType


NuGet doesn't seem to have an enum for AssetType, do we need one? We are just turning strings into this enum, and then back into strings to put on Item metadata. Is the enum necessary?

The Enum is convenient for reasoning on the types of assets we are dealing with

eerhardt · 2017-01-18T15:31:28Z

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

+        one of these files and not have an incremental build replace it.
+        -->
+    <Copy SourceFiles = "@(_ManagedResolvedFileToPublish)"
+          DestinationFiles="$(PublishDir)%(RecursiveDir)%(Filename)%(Extension)"


We fixed a bug the way Publish copies files and the bug seems to be duplicated here. See #581

I am going to copy the files every time, will remove the comments

eerhardt · 2017-01-18T15:33:22Z

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

+<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="$(MSBuildToolsVersion)">
+  <PropertyGroup>
+      <TargetFramework>$(_TFM)</TargetFramework>
+      <RuntimeIdentifier>$(RuntimeIdentifier)</RuntimeIdentifier>


This seems redundant. It is setting a property to itself.

This is used to generate the Project to restore crossgen, the Alternative is to pass it as parameter to the Restore Target. I will leave it here for increased diagnosability

ramarag · 2017-01-19T20:11:58Z

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

+
+    <!-- CrossGen the assemblies  -->
+    <Message Text="$(_Crossgen) -readytorun -in %(_AssembliestoCrossGen.Fullpath) -out $([System.IO.Path]::Combine($(_RuntimeOptimizedDir),%(RecursiveDir)%(Filename)%(Extension))) -jitpath $(_JitPath) -platform_assemblies_paths $(_RuntimeRefDir)$(PathSeparator)$(_NetCoreRefDir)"/>
+    <Exec


Currently the Exec tasks are running sequentially

@eerhardt @dsplaisted any ideas on making them run in parallel ?

http://stackoverflow.com/questions/997047/how-to-run-tasks-in-parallel-in-msbuild

AFAIK, the only way you can parallelize work in MSBuild is to split the work into separate MSBuild invocations. That was the answer 7 years ago, and I'm assuming it is still the same answer.

@rainersigwald @jeffkl @cdmihai - in case they know of a way

Running things in parallel while waiting until they are all complete is a hard enough problem as it is :) The <Exec /> task certainly can't do this and parallel-ism in MSBuild (AFAIK) is for building at the same time not executing targets or tasks.

What would work best is use a Task from a task library that can do this in parallel which would give you a lot of options including better logging, caching, etc. Maybe someone should work on a CrossGen MSBuild task?

That's correct, no way to do this within a project. You can create a new project that just does this work and invoke it multiple times in parallel, or you could invoke a target on this project multiple times in parallel.

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

ramarag commented Jan 5, 2017

View reviewed changes

ramarag commented Jan 6, 2017

View reviewed changes

dsplaisted reviewed Jan 6, 2017

View reviewed changes

eerhardt reviewed Jan 6, 2017

View reviewed changes

gkhanna79 reviewed Jan 10, 2017

View reviewed changes

ramarag mentioned this pull request Jan 12, 2017

Implicitly add PackageReference for NETStandard.Library if targeting .NET Standard #450

Merged

ramarag force-pushed the dotnet-cache branch 4 times, most recently from 0d906ac to 6afda82 Compare January 14, 2017 04:32

ramarag changed the title ~~[WIP] Dotnet cache~~ Dotnet cache Jan 14, 2017

ramarag force-pushed the dotnet-cache branch 2 times, most recently from 1cd7b0b to 2a93e9f Compare January 14, 2017 04:48

ramarag force-pushed the dotnet-cache branch from 2a93e9f to 66ac995 Compare January 14, 2017 04:52

ramarag force-pushed the dotnet-cache branch 2 times, most recently from 59ed07c to 9dc35d6 Compare January 18, 2017 03:46

eerhardt reviewed Jan 18, 2017

View reviewed changes

ramarag force-pushed the dotnet-cache branch 2 times, most recently from 51af688 to 55b7cda Compare January 18, 2017 23:00

ramarag self-assigned this Jan 19, 2017

ramarag commented Jan 19, 2017

View reviewed changes

Compose cache

6e119bc

ramarag force-pushed the dotnet-cache branch 3 times, most recently from d6dafc9 to 5e0d6e8 Compare January 21, 2017 05:52

Integration Test for ComposeCache

6043430

ramarag force-pushed the dotnet-cache branch from 5e0d6e8 to 6043430 Compare January 21, 2017 06:00

ramarag mentioned this pull request Jan 21, 2017

Parallelize run of crossgen during dot net cache #702

Closed

ramarag merged commit dc0b6ea into dotnet:future Jan 21, 2017

ramarag mentioned this pull request Jan 26, 2017

Dotnet cache dotnet/cli#5462

Merged

Dotnet cache #591

Dotnet cache #591

Conversation

ramarag commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schellap Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramarag Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsplaisted left a 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

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 Jan 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Jan 14, 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 14, 2017 • edited Loading

gkhanna79 commented Jan 14, 2017 via email

ramarag commented Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramarag commented Jan 5, 2017 •

edited

Loading

schellap Jan 9, 2017 •

edited

Loading

ramarag Jan 6, 2017 •

edited

Loading

ramarag Jan 13, 2017 •

edited

Loading

nguerrera Jan 14, 2017 •

edited

Loading

ramarag commented Jan 14, 2017 •

edited

Loading

ramarag commented Jan 17, 2017 •

edited

Loading