Using Apphost for post netcoreapp2.0 standalone activation #978

ramarag · 2017-03-11T00:07:25Z

eerhardt · 2017-03-11T00:22:17Z

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

+namespace Microsoft.NET.Build.Tasks
+{
+    /// <summary>
+    /// Embeds the App Name into the Nuetered host  


Nuetered should probably not be used here.

Definitely not. (And not just because it's misspelled)

eerhardt · 2017-03-11T00:23:59Z

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

+            var _modifiedAppHostPath = Path.Combine(destinationDirectory, $"{appbaseName}{hostExtension}");
+            _outputAppHostPath= new TaskItem(_modifiedAppHostPath);
+
+            if ( File.Exists(_modifiedAppHostPath))


?? Shouldn't we overwrite it if it already exists? What if it doesn't contain the correct information?

The info inside is the name of the app, so if we have placed a file here- there is not much to do.
Do you think a comment will make it more clearer

eerhardt · 2017-03-11T00:24:21Z

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

+
+            var appbaseName = Path.GetFileNameWithoutExtension(AppName);
+            var destinationDirectory = Path.GetFullPath(AppHostDestinationDirectoryPath);
+            var _modifiedAppHostPath = Path.Combine(destinationDirectory, $"{appbaseName}{hostExtension}");


local variables should not start with _. Private fields should.

eerhardt · 2017-03-11T00:30:17Z

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

@@ -525,15 +525,21 @@ Copyright (c) .NET Foundation. All rights reserved.
    -->

  <Target Name="RenamePublishedHost"
+          DependsOnTargets="_ComputeNETCoreBuildOutputFiles"


Why? That target is for "build", this is for "publish".

publish pipes in from build, the notion is largely artificial

eerhardt · 2017-03-11T00:34:35Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.DefaultItems.props

@@ -37,4 +37,8 @@ Copyright (c) .NET Foundation. All rights reserved.
    <PackageReference Include="Microsoft.NETCore.App" Version="$(RuntimeFrameworkVersion)" IsImplicitlyDefined="true" />
  </ItemGroup>

+  <ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and !$(RuntimeFrameworkVersion.StartsWith('1.'))">
+    <PackageReference Include="Microsoft.NETCore.DotNetAppHost" Version="$(RuntimeFrameworkVersion)" IsImplicitlyDefined="true" />


Won't this result in always referencing this package? Even if I'm a portable app?

That is the meta package, it will not resolved to any assets

It's going to bleed in to pack, solution explorer, etc. etc. cc @davidfowl

Errr, what is this thing...

What is the point of having the Microsoft.NETCore.DotNetHost package anymore? We will never use the dotnet executable from it, since standalone apps always use DotNetAppHost.

So instead of doing it this way, why don't we just put the apphost executable into the DotNetHost package?

Alternatively, change Microsoft.NETCore.App to reference Microsoft.NETCore.DotNetAppHost and remove the reference to Microsoft.NETCore.DotNetHost.

https://github.com/dotnet/core-setup/issues/1722

but that does not affect the change here in SDK

It does affect the change here in the SDK. If we configure the packages efficiently, the tooling doesn't need all this logic in it. See the issue above. In this change we are doing things like

Delete the dotnet.exe file from the published files.

First look for apphost.exe, if that isn't found fall back to dotnet.exe.

The tooling logic can be a lot simpler.

I agree it can be made simpler once we have figured the correct changes for Core-Setup. However, that should not block enabling this feature. @ramarag please context to this discussion in the issue above so that, once the feature is enabled, you should visit cleaning up Core-Setup, followed by the SDK.

@eerhardt i have already done what you are suggesting.
@gkhanna79 the latest M.N.App package indeed refers the Apphost package, but the one sdk currently refers does not, will update it to the latest version and remove this reference

Actually spoke too soon, i will have to update M.N.App to get apphost.exe in it

Getting this done by dotnet/core-setup#1728

eerhardt · 2017-03-11T00:35:59Z

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

+      <ResolvedFileToRemove  Include ="%(ResolvedFileToPublish.Identity)" Condition="'%(ResolvedFileToPublish.RelativePath)' == '$(_DotNetHostExecutableName)'"/>
+      <ResolvedFileToPublish Remove ="%(ResolvedFileToRemove.Identity)"/>
+
+      <ResolvedFileToAdd Include="%(NativeAppHostNETCore.Identity)">


Why use 2 items here? Why not just Include this into <ResolvedFileToPublish>?

Because i need to set the metadata on the item

You can set metadata on the item even without using a separate item type here:

<ResolvedFileToPublish Include="%(NativeAppHostNETCore.Identity)"> <RelativePath>$(AssemblyName)%(Extension)</RelativePath> </ResolvedFileToPublish>

Works and is simpler and more performant.

This sets the metadata on all ResolvedFileToPublish, not what we actually want

No it doesn't. Just like:

<Content Include="File1"> </Content> <Content Include="File2"> <CopyToOutputDirectory>Always</CopyToOutputDirectory> </Content>

In that scenario, how many files get copied to the output directory? Only File2. Adding a new Item and setting metadata on the item doesn't set the metadata on all the existing files in that item.

eerhardt · 2017-03-11T00:36:25Z

                                    RenamePublishedHost

This name and comment should be updated. This is no longer what this target does.

Refers to: src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Publish.targets:521 in 0203e6f. [](commit_id = 0203e6f, deletion_comment = False)

eerhardt · 2017-03-11T00:36:44Z

src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx

+  <data name="FileNameIsTooLong" xml:space="preserve">
+    <value>Given File Name '{0}' is longer than 1024 bytes</value>
+  </data>
+  <data name="HostIsNotNuetered" xml:space="preserve">


Again, please don't use Nuetered.

eerhardt · 2017-03-11T00:37:19Z

test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs

+            var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);
+
+
+            TestProject referencerProject = new TestProject()


Why is this named referencer?

oops copy paste error

eerhardt · 2017-03-11T00:37:49Z

test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs

+                IsSdkProject = true,
+                TargetFrameworks = targetFramework,
+                RuntimeFrameworkVersion = RepoInfo.NetCoreApp20Version,
+                // Need to use a self-contained app for now because we don't use a CLI that has a "2.0" shared framework


this comment doesn't belong. We use a self-contained app because thatis what we are testing.

oops copy paste error

eerhardt · 2017-03-11T00:39:39Z

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

+    <EmbedAppNameInHost   AppHostSourcePath="@(NativeRestoredAppHostNETCore)"
+                          AppHostDestinationDirectoryPath="$(AppHostDestinationDirectoryPath)"
+                          AppName="$(AssemblyName).dll"
+                          StringToReplace="foobar"


Foobar is among terms that are not to be used in products. Also, the blind search and replace strategy carries risk that the mere 5 bytes occur elsewhere in the exe with completely different meaning.

blame @schellap for this, does it make more sense to move this string into C# or just hard code the sha value ?

eerhardt · 2017-03-11T00:41:44Z

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

+
+    <EmbedAppNameInHost   AppHostSourcePath="@(NativeRestoredAppHostNETCore)"
+                          AppHostDestinationDirectoryPath="$(AppHostDestinationDirectoryPath)"
+                          AppName="$(AssemblyName).dll"


Probably shouldn't be hard-coding .dll here. If you really need the extension, use $(TargetExt). Why do we use the extension in the AppName?

This is the extension of the managed assembly, i believe it will always be a ".dll"

If it would always be a .dll, then we wouldn't have the $(TargetExt) property. We would just hard-code .dll everywhere.

Use the property.

But again, why do we put the extension in the AppName? MyApp isn't "named" MyApp.dll.

@eerhardt Please see my first comment on the review of this PR - I think it should not be called AppName but rather AppBinaryName.

nguerrera · 2017-03-11T00:39:11Z

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

+                return;
+            }
+
+            if (AppName.Length > 1024)


Shouldn't this be checked against utf8-encoded byte length?

is it not the same ?

It is not. If AppName has characters outside ASCII, it will take up more than AppName bytes.

nguerrera · 2017-03-11T00:40:12Z

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

+
+        protected override void ExecuteCore()
+        {
+            var array = File.ReadAllBytes(AppHostSourcePath);


How big is apphost? This code seems very likely to be slow.

This is a one time operation for every project. We have made the design decision to replace the byte stream in the host, i suppose there is no way out

On win7-x64 it is 74 kb

Surely we can do better than copying everything in to 74 kb managed array and doing LINQ (!) over it. See @schellap's sample. Maybe it doesn't have to be quite that sophisticated, but I think the naive approach here is pushing it.

You should move the byte[] read to be after the checks below so that it is not performed in vain.

nguerrera · 2017-03-11T00:43:10Z

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

+    <EmbedAppNameInHost   AppHostSourcePath="@(NativeRestoredAppHostNETCore)"
+                          AppHostDestinationDirectoryPath="$(AppHostDestinationDirectoryPath)"
+                          AppName="$(AssemblyName).dll"
+                          StringToReplace="foobar"


Foobar is among terms that are not to be used in products. Also, the blind search and replace strategy carries risk that the mere 5 bytes occur elsewhere in the exe with completely different meaning.

nguerrera · 2017-03-11T00:50:56Z

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

+                throw new BuildErrorException(Strings.FileNameIsTooLong, AppName);
+            }
+
+            var byteArray = SHA256.Create().ComputeHash(Encoding.UTF8.GetBytes(StringToReplace));


Oh, I missed that we're replacing the sha of foobar, not foobar. So disregard 5 byte risk part of earlier comment. Still why are we wasting cpu resources computing this fixed value every time?

nguerrera · 2017-03-11T01:03:00Z

src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx

+    <value>Given File Name '{0}' is longer than 1024 bytes</value>
+  </data>
+  <data name="HostIsNotNuetered" xml:space="preserve">
+    <value>The App host does not have the embedded hash value of string '{0}'</value>


As of now, this will report 'foobar' to user which would be a world ready tenet blocker and really embarrassing if we ever have to explain this error.

What is the point of using a hash for this marker. Can't we just generate a guid?

I think the user facing message for this should not go in to the implementation details of the replacement. This is basically an invalid (corrupted?) apphost, right?

schellap · 2017-03-11T04:01:54Z

Btw, you wouldn't be replacing the foobar in the exe. You should replace SHA-256 of foobar in the exe. Just use the SHA here (Btw, it is some opaque SHA -- called out as foobar just for the unittest and comments just in case you wanted to regenerate.)

…

On Fri, Mar 10, 2017 at 5:44 PM, Rama krishnan Raghupathy < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.targets <#978 (comment)>: > + <CopyToPublishDirectory>Never</CopyToPublishDirectory> + </AllNETCoreCopyLocalItems> + </ItemGroup> + + <PropertyGroup Condition="'@(NativeRestoredAppHostNETCore)' != '' "> + <AppHostDestinationDirectoryPath>$(BaseIntermediateOutputPath)\$(TargetFramework)\$(RuntimeIdentifier)\host</AppHostDestinationDirectoryPath> + </PropertyGroup> + + <NETSdkError Condition="'@(NativeRestoredAppHostNETCore->Count())' > 1" + ResourceName="MultipleFilesResolved" + FormatArguments="$(_DotNetAppHostExecutableName)" /> + + <EmbedAppNameInHost AppHostSourcePath="@(NativeRestoredAppHostNETCore)" + AppHostDestinationDirectoryPath="$(AppHostDestinationDirectoryPath)" + AppName="$(AssemblyName).dll" + StringToReplace="foobar" blame @schellap <https://github.com/schellap> for this, does it make more sense to move this string into C# or just hard code the sha value ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#978 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKJrs8VKfJ5MqC-LlKPzdXZe3YTbmJGKks5rkfx4gaJpZM4MaAfr> .

schellap · 2017-03-11T04:06:45Z

Also, I'd recommend a fast string replacement algorithm. I'd imagine you want `dotnet build` to be pretty fast for large projects. See the sample in the unit test: https://github.com/dotnet/core-setup/blob/67f99ac9084ac132a1764251eee1157aec610bf5/test/TestUtils/AppHostExtensions.cs You might want to do a quick perf study with a simple C# program with both implementations and choose whatever suits best.

…

On Fri, Mar 10, 2017 at 5:44 PM, Rama krishnan Raghupathy < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.targets <#978 (comment)>: > + <CopyToPublishDirectory>Never</CopyToPublishDirectory> + </AllNETCoreCopyLocalItems> + </ItemGroup> + + <PropertyGroup Condition="'@(NativeRestoredAppHostNETCore)' != '' "> + <AppHostDestinationDirectoryPath>$(BaseIntermediateOutputPath)\$(TargetFramework)\$(RuntimeIdentifier)\host</AppHostDestinationDirectoryPath> + </PropertyGroup> + + <NETSdkError Condition="'@(NativeRestoredAppHostNETCore->Count())' > 1" + ResourceName="MultipleFilesResolved" + FormatArguments="$(_DotNetAppHostExecutableName)" /> + + <EmbedAppNameInHost AppHostSourcePath="@(NativeRestoredAppHostNETCore)" + AppHostDestinationDirectoryPath="$(AppHostDestinationDirectoryPath)" + AppName="$(AssemblyName).dll" + StringToReplace="foobar" blame @schellap <https://github.com/schellap> for this, does it make more sense to move this string into C# or just hard code the sha value ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#978 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKJrs8VKfJ5MqC-LlKPzdXZe3YTbmJGKks5rkfx4gaJpZM4MaAfr> .

nguerrera · 2017-03-11T05:03:47Z

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

+            }
+
+            var placeHolder = "c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2"; //hash value embedded in apphost.exe
+            placeHolder = placeHolder.Replace("-", "").ToLower();


There are no hyphens to replace in placeholder

Nor anything to ToLower()

nguerrera · 2017-03-11T05:44:16Z

(Btw, it is some opaque SHA -- called out as foobar just for the unittest and comments just in case you wanted to regenerate.)

Why would I ever want to regenerate something opaque or why would unit test care to check that is in fact generated in any particular way? I think it will only serve to confuse folks that this is at all linked to foobar. You could have just generated 32 securely random bytes. Can we still change the marker? Policheck flags every occurrence of foobar in source code and this is just inviting folks to use it and waste cycles.

nguerrera · 2017-03-11T05:46:02Z

As far as perf, two questions:

Why not ship a tiny data file in the package that indicates the offset of the marker? No matter how clever, it seems wasteful to go hunting for it on every standalone build.
Why do we need to write the app name in to the apphost? What was wrong with it just detecting the name from whatever it gets renamed to?

nguerrera

See above

schellap · 2017-03-11T07:50:03Z

Regarding #1 Yes, @gkhanna79 and I did consider placing a file next to the exe in the nupkg, but the reasoning was: it is difficult to keep track of the contract that comes with this file and the exe no longer would be the single source of truth and `dotnet build` can definitely place a file along side the exe to memoize after first use. Regarding #2 More details are here: https://github.com/dotnet/core-proposals/pull/34. Main issue is partner signing.

…

On Fri, Mar 10, 2017 at 9:46 PM, Nick Guerrera ***@***.***> wrote: As far as perf, two questions: 1. Why not ship a tiny data file in the package that indicates the offset of the marker. No matter how clever, it seems wasteful to go hunting for it on every standalone build. 2. Why do we need to write the app name in to the apphost? What was wrong with it just detecting the name from whatever it gets renamed to? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#978 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKJrs-lm7bkLcnenz16qN-q-Jr-jUTHiks5rkjUcgaJpZM4MaAfr> .

gkhanna79 · 2017-03-13T16:37:23Z

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

+                throw new BuildErrorException(Strings.FileNameIsTooLong, AppName);
+            }
+
+            var placeHolder = "c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2"; //hash value embedded in apphost.exe


It will be good to add more comments around this and pointers to areas in Core-Setup to which this is connect. It will help the reader with more context.

gkhanna79 · 2017-03-13T16:39:23Z

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

+        public string AppHostDestinationDirectoryPath { get; set; }
+
+        [Required]
+        public string AppName { get; set; }


It is not really AppName but AppBinaryName, right? Can we reflect it in the field naming as well?

gkhanna79 · 2017-03-13T16:43:06Z

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

+            var placeHolder = "c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2"; //hash value embedded in apphost.exe
+            placeHolder = placeHolder.Replace("-", "").ToLower();
+
+            var bytesToWrite = Encoding.UTF8.GetBytes(AppName);


I would also suggest writing a comment describing what is happening in the next set of statements for the reader to get context.

gkhanna79 · 2017-03-13T16:46:45Z

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

@@ -25,7 +25,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    <_GetChildProjectCopyToPublishDirectoryItems Condition="'$(_GetChildProjectCopyToPublishDirectoryItems)' == ''">true</_GetChildProjectCopyToPublishDirectoryItems>

    <!-- publishing self-contained apps should publish the native host as $(AssemblyName).exe -->
-    <RenamePublishedHost Condition=" '$(RenamePublishedHost)' == '' and '$(OutputType)' == 'Exe' and '$(RuntimeIdentifier)' != ''">true</RenamePublishedHost>
+    <DeployAppHost Condition=" '$(DeployAppHost)' == '' and '$(OutputType)' == 'Exe' and '$(RuntimeIdentifier)' != ''">true</DeployAppHost>


Presumable, the RID check implies that this is a standalone app - is that correct @eerhardt? If so, @ramarag please add a comment around it.

Correct, here and other places in the code '$(RuntimeIdentifier)' != '' means it is a standalone app.

gkhanna79 · 2017-03-13T16:47:08Z

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


-    Renames the dotnet native host to match the application's name.
+    Deploys the host to run the app and ensures it matches the app name


We should update this to reflect that this applies to standalone apps only.

eerhardt · 2017-03-17T16:39:58Z

test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs

+
+            referencerAsset.Restore(testProject.Name);
+            var publishCommand = new PublishCommand(Stage0MSBuild, Path.Combine(referencerAsset.TestRoot, testProject.Name));
+            var publishResult = publishCommand.Execute($"/p:RuntimeIdentifier={rid}");


You don't need to pass /p:RuntimeIdentifier={rid} to publish. That value is already hard-coded into the .csproj up above. You can just say .Execute().

eerhardt · 2017-03-17T16:41:28Z

test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs

+                "Hello.pdb",
+                "Hello.deps.json",
+                "Hello.runtimeconfig.json",
+                $"{libPrefix}coreclr{Constants.DynamicLibSuffix}",


Why do we use Constants.DynamicLibSuffix, but we hard-code our own prefix?

There is no prefix in that class

eerhardt · 2017-03-17T16:41:57Z

test/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs

+                .Pass()
+                .And
+                .HaveStdOutContaining("Hello from a netcoreapp2.0.!");
+        }


Do you also want to ensure that the executable has the app name embedded in it correctly?

if it is not the apphost will fail. I was wondering should i add a knob to turn off the EmbedAppNameInHost and add a test case to turn of the knob and see the test fail, i decided against it as this is testing the host and we already do that in core-setup

ramarag · 2017-03-18T00:15:13Z

@dotnet-bot test this please

ramarag · 2017-03-18T00:15:45Z

@eerhardt @gkhanna79 @eerhardt i have addressed the comments

nguerrera · 2017-03-20T17:28:44Z

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

+        public string AppBinaryName { get; set; }
+
+        [Output]
+        public string ModifiedAppHostPath


Nit: Can be public string ModifiedAppHostPath { get; private set; }

nguerrera · 2017-03-20T17:29:29Z

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

+            get { return _modifiedAppHostPath; }
+        }
+
+        private static string placeHolder = "c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2"; //hash value embedded in default apphost executable


Nit: placeholder is one valid word so use placheholder casing (no capital H)

nguerrera · 2017-03-20T17:29:37Z

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

+
+        protected override void ExecuteCore()
+        {
+


nit: extra whitespace

nguerrera · 2017-03-20T17:30:31Z

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

+
+            SearchAndReplace(array, bytesToSearch, bytesToWrite);
+
+


nit: extra whitespace

nguerrera · 2017-03-20T17:30:52Z

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

+            SearchAndReplace(array, bytesToSearch, bytesToWrite);
+
+
+            if ( !Directory.Exists(destinationDirectory))


nit: extra whitespace next to paren

nguerrera · 2017-03-20T17:31:08Z

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

+
+                }
+            }
+


nit: extra whitespace

nguerrera · 2017-03-20T17:31:16Z

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

+            }
+
+        }
+


nit: extra whitespace

nguerrera · 2017-03-20T17:32:38Z

src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx

+    <value>The App host has been modified, it is missing the default embedded value '{0}'</value>
+  </data>
+  <data name="FileNameIsTooLong" xml:space="preserve">
+    <value>Given File Name '{0}' is longer than 1024 bytes</value>


file name (no caps, it's not a proper noun).

nguerrera · 2017-03-20T17:36:31Z

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

+            }
+
+            var array = File.ReadAllBytes(AppHostSourcePath);
+            var bytesToSearch = Encoding.UTF8.GetBytes(placeHolder);


We shouldn't need to re-encode the placeholder every time, why not just have a static readonly byte[] for that.

nguerrera · 2017-03-20T17:36:48Z

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

+                throw new BuildErrorException(Strings.FileNameIsTooLong, AppBinaryName);
+            }
+
+            var array = File.ReadAllBytes(AppHostSourcePath);


I'm still concerned about reading the entire file in to one big managed array every time.

This is a one time per project's lifetime. We can revisit it later, will file a bug for it

nguerrera · 2017-03-20T17:39:31Z

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

+        }
+
+        // See: https://en.wikipedia.org/wiki/Knuth%E2%80%93Morris%E2%80%93Pratt_algorithm
+        private static int[] ComputeKMP_FailureFunction(byte[] pattern)


Don't name non-test methods with underscores.

This doesn't need to be re-computed every time either.

Will file a bug for it

nguerrera · 2017-03-20T17:47:22Z

src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx

@@ -249,4 +249,10 @@
  <data name="RuntimeIdentifierWasNotSpecified" xml:space="preserve">
    <value>Specify a RuntimeIdentifier</value>
  </data>
+  <data name="AppHostHasBeenModified" xml:space="preserve">
+    <value>The App host has been modified, it is missing the default embedded value '{0}'</value>


This will be very rare so I think the message should help identify the problem better. Saying "modified" is a bit misleading as it could also be a bug where we get out of sync with apphost changes.

I suggest something like "Unable to use '{0}' as application host executable as it does not contain the expected placeholder byte sequence '{1}' that would mark where the application name would be written."

{0} would be the full path to the file so we can see package version, etc. if someone actually hits this and asks for support.

nguerrera · 2017-03-20T17:52:46Z

I'm OK postponing the perf changes, but I'd like an issue to be filed to look at that.

Please clean up the whitespace and the error message before merging.

* Update dependencies from https://github.com/dotnet/arcade build 20191001.4 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19501.4 * Update dependencies from https://github.com/dotnet/arcade build 20191002.11 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19502.11

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

eerhardt reviewed Mar 11, 2017

View reviewed changes

nguerrera reviewed Mar 11, 2017

View reviewed changes

ramarag force-pushed the apphost branch from 0203e6f to 232d11b Compare March 11, 2017 03:57

ramarag force-pushed the apphost branch from 232d11b to beaeec5 Compare March 11, 2017 04:02

nguerrera reviewed Mar 11, 2017

View reviewed changes

nguerrera suggested changes Mar 11, 2017

View reviewed changes

gkhanna79 reviewed Mar 13, 2017

View reviewed changes

eerhardt reviewed Mar 17, 2017

View reviewed changes

ramarag force-pushed the apphost branch from 378903b to ea5c271 Compare March 17, 2017 23:17

eerhardt approved these changes Mar 20, 2017

View reviewed changes

nguerrera reviewed Mar 20, 2017

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/EmbedAppNameInHost.cs Outdated

}

}

Copy link

Contributor

nguerrera Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra whitespace

nguerrera reviewed Mar 20, 2017

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/EmbedAppNameInHost.cs Outdated

}

}

Copy link

Contributor

nguerrera Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra whitespace

nguerrera reviewed Mar 20, 2017

View reviewed changes

nguerrera approved these changes Mar 20, 2017

View reviewed changes

ramarag force-pushed the apphost branch from ea5c271 to b94b642 Compare March 20, 2017 21:09

ramarag added 2 commits March 20, 2017 14:18

Use apphost.exe as standalone host for post 2.0 projects

ae07b95

test for post netcoreapp2.0 standalone activation

2878c47

ramarag force-pushed the apphost branch from b94b642 to 2878c47 Compare March 20, 2017 21:18

ramarag merged commit 952b5b7 into dotnet:master Mar 20, 2017

This was referenced Mar 20, 2017

Improve Cold start Build/Publish Performance for stand alone apps #1014

Closed

Using Apphost.exe for standalone apps dotnet/cli#6085

Merged

Use apphost.exe for app-base application activations #562

Closed

		var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);


		TestProject referencerProject = new TestProject()


		Renames the dotnet native host to match the application's name.
		Deploys the host to run the app and ensures it matches the app name

		SearchAndReplace(array, bytesToSearch, bytesToWrite);


		if ( !Directory.Exists(destinationDirectory))

Using Apphost for post netcoreapp2.0 standalone activation #978

Using Apphost for post netcoreapp2.0 standalone activation #978

Conversation

ramarag commented Mar 11, 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

nguerrera Mar 11, 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

eerhardt commented Mar 11, 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

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

schellap commented Mar 11, 2017 via email

schellap commented Mar 11, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Mar 11, 2017 • edited Loading

nguerrera commented Mar 11, 2017 • edited Loading

nguerrera left a comment

Choose a reason for hiding this comment

schellap commented Mar 11, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkhanna79 Mar 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

ramarag commented Mar 18, 2017

ramarag commented Mar 18, 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

nguerrera Mar 11, 2017 •

edited

Loading

nguerrera commented Mar 11, 2017 •

edited

Loading

nguerrera commented Mar 11, 2017 •

edited

Loading

gkhanna79 Mar 13, 2017 •

edited

Loading