Enable generate runtime config and deps.files #4063

wli3 · 2019-12-16T01:48:12Z

Please review it commit by commit.

wli3 · 2019-12-16T18:59:16Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets

    <NETSdkError Condition="'$(Language)' == 'C++' and $(EnableComHosting) == 'true'"
                 ResourceName="NoSupportCppEnableComHosting" />

+    <NETSdkError Condition="'$(Language)' == 'C++' and $(SelfContained) == 'true'"


Need discussion. Although Rainier suggested we could add a target in C++ or SDK to override SelfContained to false. So we can keep the existing behavior

wli3 · 2019-12-16T18:59:51Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateRuntimeConfigurationFiles.cs

+    }
+  }
+}");
+            File.Exists(_runtimeConfigDevPath).Should().BeFalse("No nuget involved, so no extra probing path");


need discussion, double check, No nuget involved, so no extra probing path, no runtimeConfigDev needed

Sounds fine to me.

wli3 · 2019-12-16T19:01:10Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets


+  <!-- To achieve incremental build with property change. When any property changes, WriteOnlyWhenDifferent will be triggered to write cache file.
+  And the cache file's timestamp will be later, and it then triggers the incremental build.-->
+  <Target Name="_GenerateRuntimeConfigurationFilesInputCache" DependsOnTargets="ResolveAssemblyReferences">


need discussion. maybe over kill, but that should be the "right thing"

Seems OK.

But why does this depend on ResolveAssemblyReferences? I don't see anything from what it does being used here.

runtimeconfig is so small, that I sort of wonder if it wound't be simpler, easier to maintain, or more efficient to just generate it and write the whole file only if different rather than any hashing that might need to be kept up to date with new properties.

Good idea. The hash is probably bigger than the file. I reverted that commit and added 31cec9d instead.

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenADependencyContextBuilder.cs

nguerrera · 2020-01-02T22:28:40Z

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateRuntimeConfigurationFiles.cs

+    }
+  }
+}");
+            File.Exists(_runtimeConfigDevPath).Should().BeFalse("No nuget involved, so no extra probing path");


Sounds fine to me.

src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAGenerateRuntimeConfigurationFiles.cs

nguerrera · 2020-01-02T22:33:02Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets


+  <!-- To achieve incremental build with property change. When any property changes, WriteOnlyWhenDifferent will be triggered to write cache file.
+  And the cache file's timestamp will be later, and it then triggers the incremental build.-->
+  <Target Name="_GenerateRuntimeConfigurationFilesInputCache" DependsOnTargets="ResolveAssemblyReferences">


Seems OK.

But why does this depend on ResolveAssemblyReferences? I don't see anything from what it does being used here.

runtimeconfig is so small, that I sort of wonder if it wound't be simpler, easier to maintain, or more efficient to just generate it and write the whole file only if different rather than any hashing that might need to be kept up to date with new properties.

wli3 · 2020-01-06T22:45:44Z

@nguerrera 31cec9d is the new commit for fix missing runtimeconfig. I resolved your comments so far.

nguerrera · 2020-01-06T22:58:39Z

I decided to check on the perf here. It seems GenerateRuntimeConfigurationFiles is a noticeable part of incremental build when assets file has changed.

You can compare dotnet build /clp:PerformanceSummary vs. touch obj\projects.assets.json && donet build /clp:PerformanceSummary

I'm doing this on a vanilla dotnet new console and seeing GenerateRutnimeConfigurationFiles move to near the most expensive.

      38 ms  GenerateDepsFile                           1 calls
       74 ms  GenerateRuntimeConfigurationFiles          1 calls
      100 ms  MsBuild                                    7 calls
      102 ms  RestoreTask                                1 calls

I am surprised by this result, but it might invalidate my idea of doing it this way.

nguerrera · 2020-01-06T22:59:35Z

What do you think about treating the perfect up-to-date-ness of runtime config as a separate change from this one?

When asset file is missing. By adding correct incremental build with property hased

wli3 · 2020-01-07T01:40:55Z

Use back incremental build fix instead. Log this for 5.0 #4136

- Rename to projectContextHasProjectReferences - Move DependsOnTargets=_DefaultMicrosoftNETPlatformLibrary - Revert unnecessary dependency

wli3 · 2020-01-07T02:13:36Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

    ============================================================
    -->
  <Target Name="_ComputeNETCoreBuildOutputFiles"
-          DependsOnTargets="_GetAppHostPaths;_GetComHostPaths;_GetIjwHostPaths;GenerateBuildRuntimeConfigurationFiles"


Reverted this

wli3 · 2020-01-07T02:15:12Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets

    -->

  <Target Name="GenerateBuildRuntimeConfigurationFiles"
-          DependsOnTargets="_DefaultMicrosoftNETPlatformLibrary;_GenerateRuntimeConfigurationFilesInputCache"


Move dependency on _DefaultMicrosoftNETPlatformLibrary to _GenerateRuntimeConfigurationFilesInputCache. So _GenerateRuntimeConfigurationFilesInputCache can get the same property as before. Some property need to be set by a target before _GenerateRuntimeConfigurationFilesInputCache runs

wli3 requested a review from nguerrera December 16, 2019 18:25

wli3 marked this pull request as ready for review December 16, 2019 18:25

wli3 commented Dec 16, 2019

View reviewed changes

wli3 force-pushed the enable-runtimeconfig-cpp-use-related-files branch from a3340fe to 1068a06 Compare December 17, 2019 00:26

livarcocc added this to the 3.1.2xx milestone Jan 2, 2020

nguerrera reviewed Jan 2, 2020

View reviewed changes

wli3 force-pushed the enable-runtimeconfig-cpp-use-related-files branch 2 times, most recently from b49bd9e to 75f522d Compare January 3, 2020 17:30

William Li added 6 commits January 6, 2020 17:37

Add test asset

b6e710e

Block self-contained with C++/CLI project

ae671d4

Add enable Deps json file without asset file

34cf1cd

Enable Generate runtime config without asset file

8ef6a83

Fix no Runtime config generated

6ac0031

When asset file is missing. By adding correct incremental build with property hased

Fix bad merge

86b84f5

wli3 force-pushed the enable-runtimeconfig-cpp-use-related-files branch from 75f522d to 902463d Compare January 7, 2020 01:38

Address code review

f8b2c41

- Rename to projectContextHasProjectReferences - Move DependsOnTargets=_DefaultMicrosoftNETPlatformLibrary - Revert unnecessary dependency

wli3 force-pushed the enable-runtimeconfig-cpp-use-related-files branch from 902463d to f8b2c41 Compare January 7, 2020 02:13

wli3 commented Jan 7, 2020

View reviewed changes

nguerrera approved these changes Jan 7, 2020

View reviewed changes

wli3 merged commit 831a360 into dotnet:release/3.1.2xx Jan 7, 2020

wli3 deleted the enable-runtimeconfig-cpp-use-related-files branch January 7, 2020 18:34

Enable generate runtime config and deps.files #4063

Enable generate runtime config and deps.files #4063

Uh oh!

Conversation

wli3 commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wli3 commented Jan 6, 2020

Uh oh!

nguerrera commented Jan 6, 2020

Uh oh!

nguerrera commented Jan 6, 2020

Uh oh!

wli3 commented Jan 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wli3 commented Dec 16, 2019 •

edited

Loading