Add ability to mark self-contained .exe as Windows GUI program. #2470

vitek-karas · 2018-08-13T09:02:11Z

On Windows only, if the OutputType of the project is WinExe (as oppose to just exe), self-contained published app will now have its subsystem set to Windows GUI.
This means the app will start without a console.

The change renames the task EmbedAppNameInHost to PrepareAppHost as it now does more than just embedding the path to the app.

Added unit tests for the AppHost class and then an E2E for the publish command.

livarcocc · 2018-08-14T00:36:03Z

@peterhuene does this conflict with your mode change?

@vitek-karas we are working on having exes for framework dependent apps as well. Should the same change be available there as well? If it is simply in your roadmap and will come in later, disregard my question.

livarcocc · 2018-08-14T00:36:31Z

src/Tasks/Common/Resources/Strings.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>


Why all the formatting changes o this file?

VS started doing this at some point. I think we should just take it because I've reverted it myself a bunch of times. Would have preferred to do it separately, but let's get it over with.

Created #2471 with just the formatting changes - will wait for it to merge.

livarcocc · 2018-08-14T00:39:08Z

src/Tasks/Common/Resources/Strings.resx

+    <comment>{StrBegin="NETSDK1071: "}</comment>
+  </data>
+  <data name="AppHostNotWindowsCLI" xml:space="preserve">
+    <value>NETSDK1072: Unable to use '{0}' as application host executable because it's not a Windows executable for teh CUI subsystem.</value>


There is a typo here. teh instead of the. Also, CUI system? Should it be GUI?

Thanks, fixed. CUI is the Windows term for console app subsystem. GUI - Graphical subsystem, CUI - Console subsystem. The apphost.exe should be prebuilt as a CUI, if not this check will fail (also there to make sure we didn't compute wrong index).
I added the (Console) into the error message to make this clearer.

vitek-karas · 2018-08-14T09:21:54Z

I only just now learned that there are similar efforts... so if this somehow collides with other people's work, please do let me know. I made this change mostly as a learning experience... and since it works just fine, made it a PR.

This seems to work even for framework dependent exes. As it should - I would not expect any differences in this regard. I tried locally building an app with /p:UseAppHost=true /p:SelfContained=false /p:OutputType=WinExe and it did produce an .exe which has the GUI bit set but otherwise seemed to work as usual.

vitek-karas · 2018-08-14T09:34:11Z

Fixes #2464

vitek-karas · 2018-08-14T09:34:37Z

Related to dotnet/core-setup#196

peterhuene · 2018-08-14T18:05:26Z

src/Tasks/Common/Resources/Strings.resx

@@ -401,4 +401,12 @@ The following are names of parameters or literal values and should not be transl
    <value>NETSDK1070: The application configuration file must have root configuration element.</value>
    <comment>{StrBegin="NETSDK1070: "}</comment>
  </data>
-</root>
+  <data name="AppHostNotWindows" xml:space="preserve">
+    <value>NETSDK1071: Unable to use '{0}' as application host executable because it's not a Windows PE file.</value>


We'll likely want to improve this UX if users are trying to build a WinExe output type project and specify a non-Windows RID, but that shouldn't prohibit merging this PR.

Currently the .targets will make it so that we effectively ignore WinExe on non-Windows RIDs. Just like we do today. I didn't want to break anything, so on Windows RIDs, we now honor the OutputType=Exe (CUI)/WinExe (GUI). On non-Windows RIDs both Exe/WinExe will behave just like now - that is the same.

Yeah, with respect to this change this looks good; in the future I think we should error with something that informs the user that this project requires a Windows RID because the output type is WinExe.

vitek-karas · 2018-08-14T18:11:34Z

@KathleenDollard for potential UX aspects.
In short, on Windows RIDs this change will make it so that OutputType=Exe produces Console apphost.exe and OutputType=WinExe produces GUI apphost.exe.

peterhuene · 2018-08-14T18:15:17Z

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

+            try
+            {
+                accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref pointer);
+                byte* bytes = pointer + accessor.PointerOffset;


Should we have sanity bounds check that on the view handle that it is at least 0x99 bytes in length?

Good point - will do. Will reuse the exception message that it's not a valid Windows PE file (since it wouldn't be :-))

I also couldn't math. Need two checks: one that the size is at least 0x40 and the other is that the NT header offset + 0x5C is within bounds.

Definitely - already working on it

peterhuene · 2018-08-14T18:17:07Z

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

@@ -272,7 +272,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    Computes any files that need to be copied to the build output folder for .NET Core.
    ============================================================
    -->
-  <UsingTask TaskName="EmbedAppNameInHost" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />
+  <UsingTask TaskName="PrepareAppHost" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />


👍 on name change, as this will also make more sense for additional 3.0 changes.

Yep, this name has been bugging me for a while. :)

nit: Although "Prepare" doesn't really mean too much. And the file is done after this target right? and it will just be copied around

I can't think of anything better right now - if you have a better name, I'll be happy to change it.

CreateAppHost might be good since it matches what we called it in the C# helper. I'm OK with Prepare too though.

peterhuene · 2018-08-14T18:18:25Z

@livarcocc This will not conflict with the SDK changes for using the apphost for framework-dependent apps, since it's just some targets logic.

@vitek-karas Changes look good, other than the one comment for a bounds check where we're modifying the PE header. Thanks!

nguerrera · 2018-08-14T18:32:25Z

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

        /// <summary>
        /// Create an AppHost with embedded configuration of app binary location
        /// </summary>
        /// <param name="appHostSourceFilePath">The path of Apphost template, which has the place holder</param>
        /// <param name="appHostDestinationFilePath">The destination path for desired location to place, including the file name</param>
        /// <param name="appBinaryFilePath">Full path to app binary or relative path to the result apphost file</param>
        /// <param name="overwriteExisting">If override the file existed in <paramref name="appHostDestinationFilePath"/></param>
+        /// <param name="customizationSettings">Optional settings to customize the creates apphost</param>


nit: typo: s/the creates apphost/the created/apphost

nguerrera · 2018-08-14T18:45:52Z

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

+        /// <summary>
+        /// Settings to customize the apphost.
+        /// </summary>
+        public class HostCustomizationSettings


Nit: I think "Customization" is redundant when we're already talking about "Settings" or "Options". I also think "Host" is redundant in nested scope with AppHost.

Even more subjective, but I also prefer "Options" over "Settings".

And finally, Framework Design Guidelines discourage public nested classes. Those don't apply here, but I like to follow this one.

I would prefer any of these (listed in my personal order of preference):

AppHostOptions (top-level)

AppHostSettings (top-level)

AppHost.Options (nested)

AppHost.Settings (nested)

I thought about making the AppHostOptions class separate (top-level), but then it would require its own file and seems way too small for that. But I will happily oblige if others would like that.

I don't mind a new file since we're likely to keep expanding the options as the 3.0 features get implemented. I'd also second AppHostOptions.

nguerrera · 2018-08-14T18:47:17Z

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

        public static void Create(
            string appHostSourceFilePath,
            string appHostDestinationFilePath,
            string appBinaryFilePath,
-            bool overwriteExisting = false)
+            bool overwriteExisting = false,
+            HostCustomizationSettings customizationSettings = null)


Continuing earlier nit, I'd rename parameter here to just settings. (Or justoptions if you do take the "Options" over "Settings" in the class name.)

nguerrera · 2018-08-14T18:48:11Z

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

@@ -272,7 +272,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    Computes any files that need to be copied to the build output folder for .NET Core.
    ============================================================
    -->
-  <UsingTask TaskName="EmbedAppNameInHost" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />
+  <UsingTask TaskName="PrepareAppHost" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />


Yep, this name has been bugging me for a while. :)

nguerrera · 2018-08-14T18:50:55Z

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

+
+                // https://en.wikipedia.org/wiki/Portable_Executable
+                // Validate that we're looking at Windows PE file
+                if (((UInt16*)bytes)[0] != 0x5A4D)


Can you put these numbers into consts? They can be local and close by first use and keeping the comments to documentation nearby as well.

nguerrera · 2018-08-14T18:51:55Z

Looks good overall, I only had nits.

peterhuene · 2018-08-14T20:21:49Z

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

+        /// <summary>
+        /// The first two bytes of a PE file are a constant signature.
+        /// </summary>
+        private const UInt16 _peFileSignature = 0x5A4D;


Nit: could we use const-naming from the guidelines? When reading the source, it makes me think these are fields.

Definitely - I just copied what was already in the class (look at the top of the file).
Just to make sure - const field would be PascalCased right?

Ugh, I didn't notice that we do it against the guideline already in this file. I'd still advocate for doing it the "correct" way here (PascalCased) :)

wli3 · 2018-08-14T21:22:46Z

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

+        private const string _placeHolder = "c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2"; //hash value embedded in default apphost executable
+        private readonly static byte[] _bytesToSearch = Encoding.UTF8.GetBytes(_placeHolder);
+
+        [Fact]


[WindowsOnlyFact] ? it used _windowsFileHeader

Sorry - I don't understand your comment.
The GUI bit only works on Windows RIDs. I guess it might be possible to run the publish on Mac for a Windows RID and it should still work... I'll have to try that somehow.

nvm, it is not real file

peterhuene · 2018-08-14T23:21:55Z

The test failures on Windows appear to be infrastructure-related. I'd rerun, but there might still be one more commit pushed if my remaining naming nit is fixed.

nguerrera · 2018-08-15T21:11:14Z

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


        // This is a dump of first 350 bytes of a windows apphost.exe
        // This includes the PE header and part of the Optional header
-        private static byte[] _windowsFileHeader = new byte[] {
+        private static byte[] WindowsFileHeader = new byte[] {


This one is technically not a constant. I would have used s_windowsFileHeader from my corefx upbringing, but we don't seem to be consistent on that and I think our editorconfig was tweaked to use just _ even for static fields.

I don't care enough about this to block the commit, though. Seems we should make a call and fix all the static fields up separately.

Commented on #1589

Actually this should be const/readonly.. I'll fix that. In which case the new naming scheme fits much better.

nguerrera · 2018-08-15T21:13:38Z

@dotnet-bot test this please

On Windows only, if the OutputType of the project is WinExe (as oppose to just exe), self-contained published app will now have its subsystem set to Windows GUI. This means the app will start without a console. The change renames the task EmbedAppNameInHost to PrepareAppHost as it now does more than just embedding the path to the app. Added unit tests for the AppHost class and then an E2E for the publish command.

Can't rely on the optional header to stay at fixed offset, so read the offset from the PE header even in tests.

- Move the options into a top-level class AppHostOptions - Use named constants for all the magic numbers - Check for buffer overflows - Fix typos

Use named constants in tests as well

Also added better description in the comment of the task.

Fix the naming in tests as well (product code has been changed in a previous commit). Also makes one of the const fields readonly as it should not be mutable.

vitek-karas · 2018-08-20T21:23:58Z

I renamed the task to CreateAppHost. Unless somebody complains, I'm going to merge this tomorrow.

vitek-karas force-pushed the MarkAppHostGUI branch from 5cccbc5 to 0370e45 Compare August 13, 2018 09:11

livarcocc requested review from peterhuene and a team August 14, 2018 00:34

livarcocc reviewed Aug 14, 2018

View reviewed changes

vitek-karas force-pushed the MarkAppHostGUI branch from 0370e45 to f71139b Compare August 14, 2018 09:10

peterhuene reviewed Aug 14, 2018

View reviewed changes

nguerrera reviewed Aug 14, 2018

View reviewed changes

vitek-karas force-pushed the MarkAppHostGUI branch from f71139b to a9b41b5 Compare August 14, 2018 18:59

nguerrera approved these changes Aug 14, 2018

View reviewed changes

peterhuene reviewed Aug 14, 2018

View reviewed changes

nguerrera mentioned this pull request Aug 14, 2018

Fix cannot publish self contain twice #2467

Merged

wli3 reviewed Aug 14, 2018

View reviewed changes

nguerrera reviewed Aug 15, 2018

View reviewed changes

nguerrera approved these changes Aug 15, 2018

View reviewed changes

peterhuene approved these changes Aug 15, 2018

View reviewed changes

vitek-karas added 3 commits August 20, 2018 03:39

Fix test failure after rebase

02da737

Can't rely on the optional header to stay at fixed offset, so read the offset from the PE header even in tests.

Fix error string per PR feedback

cd40c81

vitek-karas added 2 commits August 20, 2018 03:39

PR feedback fixes

62836fb

- Move the options into a top-level class AppHostOptions - Use named constants for all the magic numbers - Check for buffer overflows - Fix typos

Use PascalCasing for constants per PR feedback

7520adf

Use named constants in tests as well

vitek-karas force-pushed the MarkAppHostGUI branch from a1fbbae to 7520adf Compare August 20, 2018 10:42

vitek-karas added 2 commits August 20, 2018 03:48

Rename the task to CreateAppHost per PR feedback

8a96707

Also added better description in the comment of the task.

Rename const fields to use PascalCasing

f6c3fe0

Fix the naming in tests as well (product code has been changed in a previous commit). Also makes one of the const fields readonly as it should not be mutable.

vitek-karas merged commit 1b17e16 into dotnet:master Aug 21, 2018

vitek-karas deleted the MarkAppHostGUI branch August 21, 2018 10:01

nguerrera mentioned this pull request Sep 5, 2018

.NETCore apps cannot specify a Windows application manifest and other things that must go in exe #1899

Closed

StevenBonePgh mentioned this pull request Sep 4, 2020

UseWindowsForms='true' with OutputType='Exe' produces GUI instead of CUI executable #13331

Closed

Add ability to mark self-contained .exe as Windows GUI program. #2470

Add ability to mark self-contained .exe as Windows GUI program. #2470

Conversation

vitek-karas commented Aug 13, 2018

livarcocc commented Aug 14, 2018

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

vitek-karas commented Aug 14, 2018

vitek-karas commented Aug 14, 2018

vitek-karas commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas commented Aug 14, 2018

peterhuene Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene Aug 14, 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

peterhuene commented Aug 14, 2018

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 commented Aug 14, 2018

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

peterhuene commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Aug 15, 2018

vitek-karas commented Aug 20, 2018

peterhuene Aug 14, 2018 •

edited

Loading

peterhuene Aug 14, 2018 •

edited

Loading