Generate shim in build #2413

wli3 · 2018-07-18T21:20:21Z

Continue of #2366

problem i know so far

pending the solution to use cached asset file

It supports Clean target. However, it will not remove empty folder. Since different RID's shims have the same name. I need to separate them by folder. But msbuild will not remove that dotnet/msbuild#2408

wli3 · 2018-07-18T21:25:43Z

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

+
+namespace Microsoft.NET.Build.Tasks
+{
+    public sealed class GetEmbeddedApphostPaths : TaskBase


These code is copied out of GenerateShim, However, there is only around 10 lines are duplicated. And there is no critical code that will need to keep in sync. If we want to share these code, the overhead will be more than 10 lines.

wli3 · 2018-07-18T21:26:53Z

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

+    -->
+
+  <Target Name="GenerateShimsAssets"
+          AfterTargets="ResolveReferences"


Using AfterTargets is a good way to integrate into build? I need to be before IncrementalClean, so the file can be "registered"

AfterTargets is fine, with the caveat that you shouldn't do AfterTargets="Build" because of dotnet/msbuild#3345. But I generally prefer to use BeforeTargets--can you BeforeTargets="IncrementalClean"?

@rainersigwald thanks, although the task itself depends on ResolveReferences. Should I just dependson =ResolveReferences. and BeforeTargets="IncrementalClean". But I think use dependson with before/after target is kind of strange.

It's reasonable to have both BeforeTargets="IncrementalClean" DependsOnTargets="ResolveReferences". I'd go with that.

It is funny looking, but they're different relationships: BeforeTargets means "before you run that, run me first", while DependsOn means "before I run, run these first". Both can be true at once.

I would expect the logic for this target to be the same as GenerateBuildDependencyFile. We went through a few iterations on that and ended up against beforetargets=incrementalclean IIRC. It would be good to have one pattern for extra files we create.

I see, I read the comment. It makes sense. Now the problem is, how to set the timing(before or after what?) I think BeforeTargets="CopyFilesToOutputDirectory" is reasonable.

Changed to the following, I will push later, I don't want to hide the thread.

<Target Name="GenerateShimsAssets" BeforeTargets="CopyFilesToOutputDirectory" DependsOnTargets="ResolveReferences" Condition="'$(PackAsTool)' == 'true'" > <ItemGroup> <_PackAsToolShimRuntimeIdentifiers Include="$(PackAsToolShimRuntimeIdentifiers)"/> </ItemGroup> <PropertyGroup> <PackagedShimOutputRootDirectory Condition=" '$(PackagedShimOutputRootDirectory)' == '' ">$(OutDir)</PackagedShimOutputRootDirectory> </PropertyGroup> <GenerateShims DotNetAppHostExecutableNameWithoutExtension="$(_DotNetAppHostExecutableNameWithoutExtension)" PackagedShimOutputDirectory="$(PackagedShimOutputRootDirectory)/shims/$(TargetFramework)" PackageId="$(PackageId)" PackageVersion="$(Version)" ProjectAssetsFile="$(ProjectAssetsFile)" ProjectPath="$(MSBuildProjectFullPath)" ShimRuntimeIdentifiers="@(_PackAsToolShimRuntimeIdentifiers)" TargetFrameworkMoniker="$(NuGetTargetMoniker)" ToolCommandName="$(ToolCommandName)" ToolEntryPoint="$(ToolEntryPoint)" > <Output TaskParameter="EmbeddedApphostPaths" ItemName="_EmbeddedApphostPaths" /> </GenerateShims> <ItemGroup>  <FileWrites Include="@(_EmbeddedApphostPaths)"/> </ItemGroup> </Target>

wli3 · 2018-07-18T21:28:13Z

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

+        /// <summary>
+        /// Change "dir/file.exe" to "dir"
+        /// </summary>
+        internal static string GetDirectoryPathInRelativePath(string publishRelativePath)


It is a simply task, but I find the most reliable way is to copy paste from corefx. Any suggestions?
See dotnet/corefx#4208

wli3 · 2018-07-18T21:38:48Z

Incremental performance: we did a lot of work to avoid reading the assets file from json on every incremental build, we need to preserve that here. Ideally, we would not even read the assets file or regenerate a shim if the things that affect the content of shim have not changed.

I tried to directly use NativeLibraries itemgroup and I realize it includes 1 RID at most. And Generateshim need multiple RID's apphost. I think the solution could be

a. Resolve and add these assets during ResolvePackageAssets. And write them into cache.(or even just output an extra itemgroup). It should be best for perf, but it will introduce implicit coupling of the 2 tasks.

b. Add logic to detect "should rewrite shim". The complex part is, we need to detect the input params change. We needs another file to record what we write into shim last time (due to we cannot read back from shim).

We could do a and b both

When shim RID is set

wli3 · 2018-07-19T18:40:43Z

Incremental performance: we did a lot of work to avoid reading the assets file from json on every incremental build, we need to preserve that here. Ideally, we would not even read the assets file or regenerate a shim if the things that affect the content of shim have not changed.

I tried to directly use NativeLibraries itemgroup and I realize it includes 1 RID at most. And Generateshim need multiple RID's apphost. I think the solution could be

a. Resolve and add these assets during ResolvePackageAssets. And write them into cache.(or even just output an extra itemgroup). It should be best for perf, but it will introduce implicit coupling of the 2 tasks.

b. Add logic to detect "should rewrite shim". The complex part is, we need to detect the input params change. We needs another file to record what we write into shim last time (due to we cannot read back from shim).

We could do a and b both

@nguerrera what do you think of the asset file problem

wli3 · 2018-07-20T23:05:27Z

@nguerrera

I added these apphost assets to ResolvePackageAssets and cached it.
thanks to @rainersigwald I used method similar to this, to trigger incremental build when property change.

wli3 · 2018-07-31T16:58:33Z

@nguerrera ping review

nguerrera · 2018-07-31T17:28:03Z

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

@@ -61,6 +56,8 @@ public static class AppHost
                    SearchAndReplace(accessor, _bytesToSearch, bytesToWrite, appHostSourceFilePath);
                }
            }
+
+            File.SetLastWriteTimeUtc(appHostDestinationFilePath, DateTime.UtcNow);


This deserves a comment. Memory-mapped write is not updating last write time?

Added comment. And yes, from the test for incremental build. The last write time is not set at all without this line.

nguerrera · 2018-07-31T17:30:16Z

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

+{
+    public sealed class ExecutableExtension
+    {
+        public static string GetExecutableExtensionAccordingToRuntimeIdentifier(string runtimeIdentifier)


It's cumbersome to have "ExecutableExtension" in class and method. Maybe ExecutableExtension.ForRuntimeIdentifier?

I wonder if we can avoid this altogether by just preserving the apphost extension (or lack thereof). In other words, if we find apphost.exe, we make mytool.exe. If we find apphost, we make mytool. (And more theoretically, if we find apphost.future_os_extension, we make mytool.future_os_extension,)

Plus the comment below. It has problem with GetEmbeddedApphostPaths(a computer only, get where to look for these files). In this we don't require template apphost path. I think it is over kill to depend on template apphost path just to get the extension.

I renamed it to ForRuntimeIdentifier

nguerrera · 2018-07-31T17:31:29Z

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

+    {
+        public static string GetExecutableExtensionAccordingToRuntimeIdentifier(string runtimeIdentifier)
+        {
+            if (runtimeIdentifier.StartsWith("win"))


I forget what we decided was correct fo RIDs: case-insensitive or not? Pass StringComparison.Ordinal if case-sensitive and StringComparison.OrdinalIgnoreCase if case-insensitive.

fixed

insensitive. I added OrdinalIgnoreCase

nguerrera · 2018-07-31T17:34:27Z

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

+                    if (runtimeIdentifier.StartsWith("win"))
+                    {
+                        apphostName += ".exe";
+                    }


Why doesn't this use your helper?

I see now that we search for the full name, not the name without extension and the approach I thought about earlier where we just preserve the extension could be prone to breaking if there's ever an apphost.something_else in the package.

fixed, changed to use helper.

nguerrera · 2018-07-31T17:37:20Z

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

+    <WriteLinesToFile Lines="$(_GenerateShimsAssetsInputCacheHash)" File="$(_ShimInputCacheFile)" Overwrite="True" WriteOnlyWhenDifferent="True" />
+
+    <ItemGroup>
+      <FileWrites Include="$(_ShimInputCacheFile)" Condition="Exists('$(_ShimInputCacheFile)')" />


Why is there a condition here?

nguerrera · 2018-07-31T23:39:33Z

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

+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+


nit: extra newline

nguerrera · 2018-07-31T23:45:08Z

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

+        public ITaskItem[] ResolvedFileToPublishWithPackagePath { get; private set; }
+
+        private const char DirectorySeparatorChar = '\\';
+        private const char AltDirectorySeparatorChar = '/';


These are confusing me since they seem like they might just be using static Path when reading the code. These names are generally OS-specific. How about rename to ForwardSeparatorChar and BackwardSeparatorChar. Or WindowsSeparatorChar and UnixSeparatorChar?

fixed, renamed to ForwardSeparatorChar and BackwardSeparatorChar

nguerrera · 2018-07-31T23:52:36Z

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

+      <!-- Do this in an ItemGroup instead of as an output parameter so that it still gets added to the item set
+           during incremental builds when the task is skipped -->
+      <FileWrites Include="@(_EmbeddedApphostPaths)" Condition="Exists('@(_EmbeddedApphostPaths)')"/>
+      <FileWrites Include="$(_ShimCreatedSentinelFile)" Condition="Exists('$(_ShimCreatedSentinelFile)')"/>


I didn't know that Exists works on items. What does it return if one exists, but another doesn't.

I don't think it's necessary to condition adds to filewrites

removed condition

nguerrera · 2018-07-31T23:53:50Z

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

+         LastWriteTime in shims are not accurate. And _ShimInputCacheFile will have later timestamp than generated shims.
+         Use a created file to "record" LastWriteTime. And only use it in "Outputs" field for timestamp comparison.
+    -->
+    <WriteLinesToFile Lines="This file's LastWriteTime is used in incremental build" File="$(_ShimCreatedSentinelFile)" Overwrite="True" />


Consider using Touch task for this, which is more idiomatic in msbuild for this purpose. You'd lose this comment in the file, but I don't think we need it.

Touch uses the same corefx api that is having the bug. Once it is fixed, we can remove all this.
https://github.com/Microsoft/msbuild/blob/4972ebc6d2b81c5e967cd3260943a95c8e5eb4be/src/Tasks/Touch.cs#L133

No change here

Can you just add a comment that Touch uses the same API and therefore has the same problem.

nguerrera · 2018-08-01T00:05:36Z

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

+    /// 1. Publish PublishRelativePath is a relative <strong>file</strong> path. This need to convert to relative path.
+    /// 2. Due to DotnetTools package format, "tools/{TargetFramework}/any/" is needed in addition to the relative path.
+    /// </summary>
+    public sealed class PublishRelativePathToPackPackagePath : TaskBase


This name sounds like it's going to publish something and also doesn't make it very clear that it's specific to getting the paths for a DotnetTools package

fixed, Renamed to RelativePathFormatInPublishToFormatInPackPackage

- Add comment for setting SetLastWriteTimeUtc - Rename and ignore rid casing - Use ExecutableExtension.ForRuntimeIdentifier - Rename and remove condition on include - Rename to RelativePathFormatInPublishToFormatInPackPackage

nguerrera · 2018-08-02T19:54:35Z

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

    <ItemGroup>
      <_GeneratedFiles Include="$(PublishDepsFilePath)"/>
      <_GeneratedFiles Include="$(PublishRuntimeConfigFilePath)"/>
      <_GeneratedFiles Include="$(_ToolsSettingsFilePath)"/>
    </ItemGroup>

+    <RelativePathFormatInPublishToFormatInPackPackage


This name is still confusing me. Can it be something like ResolveToolPackagePaths?

nguerrera · 2018-08-02T19:55:10Z

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

+         LastWriteTime in shims are not accurate. And _ShimInputCacheFile will have later timestamp than generated shims.
+         Use a created file to "record" LastWriteTime. And only use it in "Outputs" field for timestamp comparison.
+    -->
+    <WriteLinesToFile Lines="This file's LastWriteTime is used in incremental build" File="$(_ShimCreatedSentinelFile)" Overwrite="True" />


Can you just add a comment that Touch uses the same API and therefore has the same problem.

wli3 commented Jul 18, 2018

View reviewed changes

wli3 requested a review from nguerrera July 18, 2018 21:40

William Li added 4 commits July 19, 2018 10:43

Generate shim in build

b624bd3

When shim RID is set

dotnet clean can remove shims

c39be55

Remove AfterTargets

967c8b6

Do not use output parameter for clean

63bb7e2

wli3 force-pushed the generate-shim-in-build branch from 04ee4bf to 63bb7e2 Compare July 19, 2018 17:50

wli3 changed the base branch from release/2.1.4xx to release/2.2.1xx July 19, 2018 17:50

wli3 added this to the 2.2.1xx milestone Jul 19, 2018

Use asset file cache and enable incremental build

2463f83

wli3 changed the title ~~[WIP] Generate shim in build~~ Generate shim in build Jul 20, 2018

William Li added 2 commits July 25, 2018 19:50

Add workaround for setLastWriteTime precision

8e981dc

Add params to hash

0924e69

nguerrera reviewed Jul 31, 2018

View reviewed changes

nguerrera reviewed Aug 1, 2018

View reviewed changes

Address code review

e5309b8

- Add comment for setting SetLastWriteTimeUtc - Rename and ignore rid casing - Use ExecutableExtension.ForRuntimeIdentifier - Rename and remove condition on include - Rename to RelativePathFormatInPublishToFormatInPackPackage

nguerrera approved these changes Aug 2, 2018

View reviewed changes

William Li added 2 commits August 2, 2018 13:59

Rename to ResolveToolPackagePaths

daaf4c0

Added comment for why touch task cannot be used

cfa08bf

wli3 merged commit 9585930 into dotnet:release/2.2.1xx Aug 3, 2018

wli3 deleted the generate-shim-in-build branch August 3, 2018 16:35

nguerrera mentioned this pull request Aug 14, 2018

Fix cannot publish self contain twice #2467

Merged

natemcmaster mentioned this pull request Oct 1, 2018

Consistently use PackagedShimOutputRootDirectory to set the path to generated shims #2561

Merged

wli3 mentioned this pull request Jan 15, 2019

Do no run GenerateShimsAssets when PackAsToolShimRuntimeIdentifiers is not set #2826

Merged

		// Copyright (c) .NET Foundation and contributors. All rights reserved.
		// Licensed under the MIT license. See LICENSE file in the project root for full license information.

Generate shim in build #2413

Generate shim in build #2413

Conversation

wli3 commented Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

wli3 Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wli3 commented Jul 18, 2018 • edited Loading

wli3 commented Jul 19, 2018

wli3 commented Jul 20, 2018

wli3 commented Jul 31, 2018

Choose a reason for hiding this comment

wli3 Aug 1, 2018 • 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

wli3 Aug 1, 2018 • 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

wli3 commented Jul 18, 2018 •

edited

Loading

wli3 Jul 18, 2018 •

edited

Loading

wli3 Jul 18, 2018 •

edited

Loading

wli3 Jul 18, 2018 •

edited

Loading

wli3 commented Jul 18, 2018 •

edited

Loading

wli3 Aug 1, 2018 •

edited

Loading

wli3 Aug 1, 2018 •

edited

Loading