Replace the check for Executable using the new variable HasRuntimeOutput and _IsExecutable #1178

basoundr · 2017-05-04T18:32:36Z

This variable is now used in places where we check to see if the project is an executable to
enable Auto Binding Redirect generation and F5 run.

This is in response to #1174

Tagging @nguerrera @srivatsn @davkean @dotnet/project-system for review

nguerrera · 2017-05-04T18:51:24Z

@eerhardt @terrajobst This aligns with the discussion on #1070

nguerrera · 2017-05-04T18:51:54Z

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

  <UsingTask TaskName="GetNearestTargetFramework" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />
  <UsingTask TaskName="NETSdkError" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />
-  
+


nit: unnecessary whitespace diff

nguerrera · 2017-05-04T18:54:40Z

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

-
-    <!-- 
+    <IsExecutable Condition="'$(IsExecutable)' == '' and '$(OutputType)'=='Exe'">true</IsExecutable>
+    <IsExecutable Condition="'$(IsExecutable)' == '' and '$(OutputType)'=='WinExe'">true</IsExecutable>


I believe this is after the use in BeforeCommon.targets and so the change won't have the effect you want.

Mind you, I'm surprised this test didn't fail:

sdk/test/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildADesktopExe.cs

Line 351 in 1d81050

public void It_generates_binding_redirects_if_needed()

I'm also surprised to see it doesn't check the contents of the app.config. Can you check that before and after your change?

Nevermind, it did fail. :)

We should move this to BeforeCommon.targets to fix it.

nguerrera

See above.

nguerrera · 2017-05-04T19:02:15Z

LGTM in principle once the ordering issue is fixed and tests pass.

eerhardt · 2017-05-04T19:07:46Z

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

@@ -304,7 +304,7 @@ Copyright (c) .NET Foundation. All rights reserved.
      </PropertyGroup>
    </When>

-    <When Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(OutputType)' == 'Exe'">
+    <When Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(IsExecutable)' == 'true'">


I was under the assumption that things like Test projects were going to set $(IsExecutable) to true. If that's the case, then I don't think these two lines should be changed.

The "Run Information" is used during F5 and dotnet run. If you try to dotnet run a Test project, will it try to execute the Test's $(TargetPath), which is a .dll on .NET Framework. Thus failing with a bad error.

Also, if Test projects are expected to set this property, I think we should think about the best name to use for this property.

@eerhardt, I would think this would be valid and part of setting IsExecutable=true would involve having uses configure the F5 information (to use xunit, nunit, dotnet, etc...)

However, maybe having a separate variable to disable (or explicitly enable) F5 for some scenarios would be desirable

@eerhardt

I was under the assumption that things like Test projects were going to set $(IsExecutable) to true.

Yes, thats true.

If that's the case, then I don't think these two lines should be changed.

We still need it so the Windows Application output type projects can still be run

I think we should think about the best name to use for this property.

I am open for a better name. But I feel IsExecutable conveys its purpose to the point.

Test projects aren't "executable". You can't double-click on it's output and expect the test to run. You can't type into a command-line C:\mytest\mytest.dll and watch the tests run.

I am open for a better name.

The list I came up with before (which we can add more to) was:

TreatAsExecutable

HasRuntimeOutput

HasRunnableOutput

HasExecutableOutput

IsExecutable

IsRunnable

I think the one I like most, given all the situations this is going to be used, is what we landed on in the project.json world: HasRuntimeOutput. The output isn't "runnable" or "executable", but instead the output is associated with a runtime, and that's why we can generate binding redirects and deps.json files that will work at runtime.

I would think this would be valid and part of setting IsExecutable=true would involve having uses configure the F5 information (to use xunit, nunit, dotnet, etc...)

That's exactly my point. Look at the rest of the code around this area that I commented on. If IsExecutable=true on a test project, the "Run Information" being returned says that you can just run the output directly:

<PropertyGroup> <RunCommand Condition="'$(RunCommand)' == ''">$(TargetPath)</RunCommand> <RunArguments Condition="'$(RunArguments)' == ''">$(StartArguments)</RunArguments>

However, maybe having a separate variable to disable (or explicitly enable) F5 for some scenarios would be desirable

Again, that's what the RunCommand property is for. If it is set, you can dotnet run or F5 this project. If it isn't set, then you can't.

Thanks, Eric. Here's what I propose based on your feedback.

_IsExecutable : "private" variable that's true iff output type is exe or winexe. Use this everywhere we have OutputType == Exe. This is not an extensibility point, it is just for our own maintainability so that we don't reintroduce winexe bugs. Controls getting a run command that just launches the output exe.

HasRuntimeOutput: defaults to _IsExecutable, but overridable by test project or similar. Controls binding redirects and other runtime configuration file generation.

If something other than a (Win)Exe wants to be runnable, it supplies a custom RunCommand as it already works.

Sorry, I meant to reply to this earlier.

This sounds like a great plan. Thanks.

srivatsn · 2017-05-04T19:16:54Z

@codito @sbaid - This PR is introducing a IsExecutable property to denote a project as executable. With this, I think you can set IsExecutable=true in your project and set OutputType=dll. That way, I think you don't have to do the Main method generation but still have deps file be generated. Would that work?

eerhardt · 2017-05-05T21:48:08Z

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

@@ -18,7 +18,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 -->
-    <DeployAppHost Condition=" '$(DeployAppHost)' == '' and '$(OutputType)' == 'Exe' and '$(RuntimeIdentifier)' != '' and '$(SelfContained)' == 'true'">true</DeployAppHost>
+    <DeployAppHost Condition=" '$(DeployAppHost)' == '' and '$(HasRuntimeOutput)' == 'true' and '$(RuntimeIdentifier)' != '' and '$(SelfContained)' == 'true'">true</DeployAppHost>


This should stay '$(OutputType)' == 'Exe' or $(_IsExecutable).. Tests won't get a $Test.exe.

eerhardt · 2017-05-05T21:50:10Z

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

@@ -210,7 +210,7 @@ Copyright (c) .NET Foundation. All rights reserved.
    it requires a host in the output directory to load the app.
    During "publish", all required assets are copied to the publish directory.
    -->
-    <ItemGroup Condition="'$(SelfContained)' == 'true' and '$(RuntimeIdentifier)' != '' and '$(OutputType)' == 'Exe'">
+    <ItemGroup Condition="'$(SelfContained)' == 'true' and '$(RuntimeIdentifier)' != '' and '$(HasRuntimeOutput)' == 'true'">


Should stay '$(OutputType)' == 'Exe' or $(_IsExecutable).

Seems _IsExecutable is the one we are looking for since we need to handle Winexe here as well

My comment was a little vague. I meant this should either:

stay '$(OutputType)' == 'Exe'
or

use $(_IsExecutable)

I didn't mean the MSBuild code should be '$(OutputType)' == 'Exe' or $(_IsExecutable)

I want to use $(_IsExecutable) in all these cases unless there's a reason to exclude WinExe but I don't think there is any such case. I think every case we did not treat exe and winexe the same was a bug.

eerhardt · 2017-05-05T21:50:14Z

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

@@ -255,7 +255,7 @@ Copyright (c) .NET Foundation. All rights reserved.
      </NativeNETCoreCopyLocalItems>
    </ItemGroup>

-    <ItemGroup Condition="'$(SelfContained)' == 'true' and '$(RuntimeIdentifier)' != '' and '$(OutputType)' == 'Exe'">
+    <ItemGroup Condition="'$(SelfContained)' == 'true' and '$(RuntimeIdentifier)' != '' and '$(HasRuntimeOutput)' == 'true'">


Should stay '$(OutputType)' == 'Exe' or $(_IsExecutable).

Seems _IsExecutable is the one we are looking for since we need to handle Winexe here as well

eerhardt · 2017-05-05T21:51:28Z

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

@@ -288,7 +288,7 @@ Copyright (c) .NET Foundation. All rights reserved.
      </PropertyGroup>
    </When>

-    <When Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(IsExecutable)' == 'true'">
+    <When Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(HasRuntimeOutput)' == 'true'">


Should stay '$(OutputType)' == 'Exe' or $(_IsExecutable). I discussed this prevously that tests are not dotnet run able, becuase you can't just run the output assembly.

Using _IsExecutable since that was original intent of this PR

eerhardt · 2017-05-05T21:51:45Z

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

@@ -304,7 +304,7 @@ Copyright (c) .NET Foundation. All rights reserved.
      </PropertyGroup>
    </When>

-    <When Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(IsExecutable)' == 'true'">
+    <When Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(HasRuntimeOutput)' == 'true'">


Should stay '$(OutputType)' == 'Exe' or $(_IsExecutable).

Using _IsExecutable since that was original intent of this PR

eerhardt · 2017-05-05T22:53:33Z

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

@@ -13,6 +13,9 @@ Copyright (c) .NET Foundation. All rights reserved.

  <PropertyGroup>
    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
+    <_IsExecutable Condition="'$(_IsExecutable)' == '' and '$(OutputType)'=='Exe'">true</_IsExecutable>
+    <_IsExecutable Condition="'$(_IsExecutable)' == '' and '$(OutputType)'=='WinExe'">true</_IsExecutable>
+    <HasRuntimeOutput Condition="'$(HasRuntimeOutput)' == ''">_IsExecutable</HasRuntimeOutput>


This should be

<HasRuntimeOutput Condition="'$(HasRuntimeOutput)' == ''">$(_IsExecutable)</HasRuntimeOutput>

eerhardt · 2017-05-05T22:55:44Z

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

@@ -33,7 +33,7 @@ Copyright (c) .NET Foundation. All rights reserved.
  </PropertyGroup>

  <PropertyGroup>
-    <GenerateRuntimeConfigurationFiles Condition=" '$(GenerateRuntimeConfigurationFiles)' == '' and '$(OutputType)' == 'exe' ">true</GenerateRuntimeConfigurationFiles>
+    <GenerateRuntimeConfigurationFiles Condition=" '$(GenerateRuntimeConfigurationFiles)' == '' and '$(HasRuntimeOutput)' == 'true' ">true</GenerateRuntimeConfigurationFiles>


This should actually have a Is .NET Core check on it too. RuntimeConfigurationFiles don't make sense for desktop or mono.

But I don't think you have to make the change here. Someone should at some point.

Opened #1183

basoundr · 2017-05-08T16:43:39Z

@dotnet-bot retest this please

eerhardt

Thanks, @basoundr.

basoundr · 2017-05-08T22:42:25Z

@dotnet-bot retest Windows_NT_FullFramework Release please

nguerrera · 2017-05-09T17:27:04Z

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

@@ -403,7 +403,7 @@ Copyright (c) .NET Foundation. All rights reserved.
  ============================================================
  -->

-  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(OutputType)' == 'Exe'">
+  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(HasRuntimeOutput)' == 'true'">
    <ProjectCapability Include="CrossPlatformExecutable" />


This will light up the docker integration in VS, which I don't think should happen for test projects once they switch to not being actual exes. @codito, @saurabsa Thoughts?

nguerrera · 2017-05-09T17:27:51Z

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

-    
-    <!-- 
+
+    <!--


Nit: This file has only whitespace diffs. Better to rebase them away to keep the history clean.

nguerrera · 2017-05-09T17:30:05Z

@dotnet-bot Test Windows_NT_FullFramework Release

…sent if the project's output has a runtime associated with it _IsExecutable is set to true iff OutputType is Exe or WinExe. This is used to avoid bugs which does not consider WinExe output. Replace the check for Executable using the new variable IsExecutable HasRuntimeOutput is now used in places where we check to see if the project is an executable to enable Auto Binding Redirect generation, deps.json and F5 run.

codito · 2017-05-10T01:49:26Z

@codito @sbaid - This PR is introducing a IsExecutable property to denote a project as executable. With this, I think you can set IsExecutable=true in your project and set OutputType=dll. That way, I think you don't have to do the Main method generation but still have deps file be generated. Would that work?

@srivatsn, @basoundr yes, we can remove the entrypoint generation code and binding redirection logic as well. I think just setting HasRuntimeOutput and OutputType=dll may not work, because of this check; we can set _IsExecutable also, but it seems like a private property.

basoundr · 2017-05-11T18:30:48Z

@dotnet-bot Test Windows_NT_FullFramework Release

livarcocc · 2017-05-11T18:57:57Z

@MattGertz for approval.

basoundr · 2017-05-11T21:20:03Z

@codito

I think just setting HasRuntimeOutput and OutputType=dll may not work

The condition for deps generation and Binding redirects are based on the value of HasRuntimeOutput. So, as long as you set the value of HasRuntimeOutput to true, everything should just work.

…224.1 (dotnet#1178) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19624.1

dnfclas added the cla-already-signed label May 4, 2017

nguerrera reviewed May 4, 2017

View reviewed changes

nguerrera mentioned this pull request May 4, 2017

Check the targetext to determine if a framework project can be run on F5 #1176

Closed

nguerrera suggested changes May 4, 2017

View reviewed changes

eerhardt reviewed May 4, 2017

View reviewed changes

basoundr changed the title ~~Replace the check for Executable using the new variable IsExecutable~~ Replace the check for Executable using the new variable HasRuntimeSupport and _IsExecutable May 5, 2017

basoundr changed the title ~~Replace the check for Executable using the new variable HasRuntimeSupport and _IsExecutable~~ Replace the check for Executable using the new variable HasRuntimeOutput and _IsExecutable May 5, 2017

eerhardt requested changes May 5, 2017

View reviewed changes

eerhardt reviewed May 5, 2017

View reviewed changes

basoundr force-pushed the IsExecutable branch from e026fa4 to 8558194 Compare May 5, 2017 23:13

basoundr mentioned this pull request May 5, 2017

GenerateRuntimeConfigurationFiles is for .Net Core Projects only #1183

Closed

eerhardt approved these changes May 8, 2017

View reviewed changes

nguerrera reviewed May 9, 2017

View reviewed changes

nguerrera approved these changes May 9, 2017

View reviewed changes

basoundr force-pushed the IsExecutable branch from 93b7554 to 424332b Compare May 9, 2017 23:06

basoundr force-pushed the IsExecutable branch from 424332b to 61f90df Compare May 9, 2017 23:08

codito mentioned this pull request May 10, 2017

Use executable flags in Microsoft.NET.Test.Sdk microsoft/vstest#792

Closed

livarcocc added this to the 2.0 milestone May 11, 2017

livarcocc added the Pending Shiproom Approval label May 11, 2017

MattGertz added Approved to merge and removed Pending Shiproom Approval labels May 11, 2017

basoundr merged commit 4309056 into dotnet:master May 11, 2017

basoundr deleted the IsExecutable branch May 11, 2017 20:12

This was referenced May 11, 2017

Explicit RID still required for .NET Framework test projects #909

Closed

Binding redirects should be enabled by default when target .NET Framework #1070

Closed

srivatsn mentioned this pull request May 12, 2017

Cannot run if Windows Application is selected dotnet/project-system#2033

Closed

mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020

Update dependencies from https://github.com/dotnet/arcade build 20191…

d9c9677

…224.1 (dotnet#1178) - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19624.1

		<UsingTask TaskName="GetNearestTargetFramework" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />
		<UsingTask TaskName="NETSdkError" AssemblyFile="$(MicrosoftNETBuildTasksAssembly)" />

Replace the check for Executable using the new variable HasRuntimeOutput and _IsExecutable #1178

Replace the check for Executable using the new variable HasRuntimeOutput and _IsExecutable #1178

Uh oh!

Conversation

basoundr commented May 4, 2017

Uh oh!

nguerrera commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguerrera May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nguerrera left a comment

Choose a reason for hiding this comment

Uh oh!

nguerrera commented May 4, 2017

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!

Choose a reason for hiding this comment

Uh oh!

srivatsn commented May 4, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basoundr commented May 8, 2017

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

basoundr commented May 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nguerrera commented May 9, 2017

Uh oh!

nguerrera May 4, 2017 •

edited

Loading

codito commented May 10, 2017 •

edited

Loading