Multitarget our msbuild task #24672

jaredpar · 2018-02-06T18:26:40Z

Changes our task to multi-target between net46 and netcoreapp2.0. This
matches how the rest of the SDK is deploying and makes for a smoother
integration into CLI.

context #24646

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

jaredpar · 2018-02-08T20:36:58Z

CC @agocke, @nguerrera, @dotnet/roslyn-infrastructure this is ready for review now

jaredpar · 2018-02-08T20:37:30Z

src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs

@@ -40,7 +40,7 @@ public override bool Execute()

            if (File.Exists(DestinationPath))
            {
-                Guid source;
+                var source = Guid.Empty;


Bonus points if someone can guess why I had to do this.

Because compiler can see private struct fields in new TFM?

Close ... it's because the definition of Guid is different between netstandard1.3 and net46 according to the reference assemblies. In one case it's a struct without fields, in the other it's a struct with private fields.

(That's basically what I meant.)

jaredpar · 2018-02-08T20:38:21Z

src/NuGet/Microsoft.NETCore.Compilers.nuspec

-    <!-- N.B.: The backslashes below cannot be replaced with forward slashes. 
-         https://github.com/NuGet/Home/issues/3584 -->
-    <file src="Dlls\MSBuildTask\netstandard1.3\publish\System.*.dll" target="tools" />
-    <file src="Dlls\MSBuildTask\netstandard1.3\publish\runtimes\**" target="tools\runtimes" />


@nguerrera, @agocke please make sure to verify the last two <file> entries I deleted here is the right decision.

Doesn't look right. You still need to carry the dlls that are not in netcoreapp2. What's left in the netcoreapp 2 publish folder?

I expect pipes.accesscontrol.

I was just including the output of build into the NuGet. Didn't think publish was necessary.

Nope, publish is necessary.

nguerrera · 2018-02-08T21:00:57Z

build/scripts/build.ps1

@@ -249,8 +250,6 @@ function Build-ExtraSignArtifacts() {
        Run-MSBuild "..\Compilers\VisualBasic\vbc\vbc.csproj" "/p:TargetFramework=netcoreapp2.0 /t:PublishWithoutBuilding"
        Write-Host "Publishing VBCSCompiler"
        Run-MSBuild "..\Compilers\Server\VBCSCompiler\VBCSCompiler.csproj" "/p:TargetFramework=netcoreapp2.0 /t:PublishWithoutBuilding"
-        Write-Host "Publishing MSBuildTask"
-        Run-MSBuild "..\Compilers\Core\MSBuildTask\MSBuildTask.csproj" "/p:TargetFramework=netstandard1.3 /t:PublishWithoutBuilding"


I would expect this to still be needed with netcoreapp2 tfm

agocke · 2018-02-08T21:19:31Z

src/Compilers/Core/MSBuildTask/MSBuildTask.csproj

-    <PackageReference Include="System.Diagnostics.Tools" Version="$(SystemDiagnosticsToolsVersion)" />
-    <PackageReference Include="System.IO.FileSystem" Version="$(SystemIOFileSystemVersion)" />
-    <PackageReference Include="System.IO.FileSystem.DriveInfo" Version="$(SystemIOFileSystemDriveInfoVersion)" />
-    <PackageReference Include="System.IO.Pipes" Version="$(SystemIOPipesVersion)" />


System.IO.Pipes is in net46 and netcoreapp2.0 by default?

That appears to be the case.

agocke · 2018-02-08T21:20:55Z

src/NuGet/Microsoft.NETCore.Compilers.nuspec

-    <!-- N.B.: The backslashes below cannot be replaced with forward slashes. 
-         https://github.com/NuGet/Home/issues/3584 -->
-    <file src="Dlls\MSBuildTask\netstandard1.3\publish\System.*.dll" target="tools" />
-    <file src="Dlls\MSBuildTask\netstandard1.3\publish\runtimes\**" target="tools\runtimes" />


Nope, publish is necessary.

agocke · 2018-02-08T21:21:53Z

src/Compilers/Shared/BuildServerConnection.cs

@@ -472,7 +472,7 @@ private static bool CheckPipeConnectionOwnership(NamedPipeClientStream pipeStrea
            }
        }

-#if NETSTANDARD1_3
+#if NET46


I would have expected far more of these #if's. Did you search to make sure there are no other mentions of NETSTANDARD1_3?

Same. There just weren't that many in the core compiler code base.

jaredpar · 2018-02-08T21:33:16Z

@agocke, @nguerrera added back the publish

nguerrera · 2018-02-08T22:09:41Z

roslyn/src/Compilers/Core/MSBuildTask/AssemblyResolution.cs

Lines 127 to 129 in c540e07

    
           var taskDir = Path.GetDirectoryName(s_assemblyLocation); 
        
           var osId = PlatformInformation.IsWindows ? "win" : "unix"; 
        
           var runtimeDir = Path.Combine(taskDir, "runtimes", osId, "lib", "netstandard1.3");

Is this still the right path?

jaredpar · 2018-03-01T18:05:31Z

@nguerrera yes that's still the right path. I did a local build and verified the output path matches.

C:\Users\jaredpar\code\roslyn\Binaries\Debug\Dlls\MSBuildTask\netcoreapp2.0\publish\runtimes\win\lib\netstandard1.3

Changes our task to multi-target between net46 and netcoreapp2.0. This matches how the rest of the SDK is deploying and makes for a smoother integration into CLI. context dotnet#24646

Now that we are multi-targeting there are a few references which are no longer necessary in the compiler package.

jaredpar · 2018-03-01T18:06:41Z

@agocke responded to your feedback.

agocke · 2018-03-02T00:49:27Z

src/NuGet/Microsoft.NETCore.Compilers.nuspec

    <!-- N.B.: The backslashes below cannot be replaced with forward slashes. 
         https://github.com/NuGet/Home/issues/3584 -->
-    <file src="Dlls\MSBuildTask\netstandard1.3\publish\System.*.dll" target="tools" />


What's the logic behind the removal of the System assemblies? This is meant to cover the cases where we bring in dependencies (like S.R.M or S.C.I) that are newer than the versions available in the chosen CLI.

The reason is that they are simply no longer present. Now that we've properly multi-targetted this task the single and only DLL present after publish is the pipes DLL.

Isn't that just luck? That is, the version of SRM/SCI that we reference is present in netcoreapp2.0. But if we upgrade to a newer one, shouldn't the new reference assembly be copied out? Otherwise I don't see how it could work properly.

I agree it won't copy those if we change them. But I don't see that as a negative here. It forces us to be explicit about the dependencies we take here.

Note: the uses of System.* was IMHO equally as wrong. Not every facade begins with System. For example Microsoft.Win32. Same argument could be made for that as S.C.I.

Isn't the point of publish that determining the runtime dependencies via inspection can be intractable? It seems like if we go this route we should also have some way of failing the build if extra files do get generated, to make sure we don't drop them on the floor, only for the compiler to fail at runtime.

I think the loading logic for CoreCLR always prefers DLL loads next to the requesting assembly

This is true for applications. In this case we are a plugin to an application. Does the logic still hold there?

This is why we only have redirect logic for desktop and for the runtimes/ directory.

What is the recommendation for the desktop case then: blindly publish or not? If it's blindly publish then how do you reconcile that with the idea that we may have to probe for versions?

What do you think the proper entry is here?

Here are the entries I considered and why none of them work:

netcoreapp2.0/publish/**: unnecessarily brings in PDB and XML files

netcoreapp2.0/publish/System.*: works for only part of the facades out there.

netcoreapp2.0/publish/System.* + microsoft.*: this double writes our own binaries in the output folder

Take everything publish puts out and just trust that it's right.

The problem is that I'm looking at the output and it doesn't seem right. The publish directory contains items we don't want (at least that I can see).

OK, you've convinced me that (1) isn't really an option either. 😄 How should we handle the case that we upgrade assemblies and new stuff needs to be published? Manual review?

I think the only thing we can do is manual update + prioritize bootstrapping out of our NuPkg files. That provides some automated checks that we have the right items there.

#24456

jaredpar requested review from a team as code owners February 6, 2018 18:26

jaredpar force-pushed the fix-msbuild branch from 9aba0da to cad146a Compare February 6, 2018 18:33

jaredpar changed the title ~~WIP: Multitarget our msbuild task~~ Multitarget our msbuild task Feb 8, 2018

jaredpar commented Feb 8, 2018

View reviewed changes

nguerrera reviewed Feb 8, 2018

View reviewed changes

agocke requested changes Feb 8, 2018

View reviewed changes

jaredpar added 6 commits March 1, 2018 10:05

Multitarget our msbuild task

d5eda94

Changes our task to multi-target between net46 and netcoreapp2.0. This matches how the rest of the SDK is deploying and makes for a smoother integration into CLI. context dotnet#24646

Remove unneeded package references

7b754d4

Now that we are multi-targeting there are a few references which are no longer necessary in the compiler package.

Fix bootstrap publish for msbuild

db22995

Updated the nuspec file

f2f86ae

Fix more missing dlls

f91f655

Put back the MSBuild publish

ed6f523

jaredpar force-pushed the fix-msbuild branch from 14995cf to ed6f523 Compare March 1, 2018 18:06

agocke reviewed Mar 2, 2018

View reviewed changes

agocke approved these changes Mar 5, 2018

View reviewed changes

jaredpar merged commit 2424421 into dotnet:dev15.7.x Mar 5, 2018

jaredpar deleted the fix-msbuild branch March 5, 2018 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multitarget our msbuild task #24672

Multitarget our msbuild task #24672

jaredpar commented Feb 6, 2018

jaredpar commented Feb 8, 2018

jaredpar Feb 8, 2018

nguerrera Feb 8, 2018

jaredpar Feb 8, 2018

nguerrera Feb 8, 2018

jaredpar Feb 8, 2018

nguerrera Feb 8, 2018

nguerrera Feb 8, 2018

jaredpar Feb 8, 2018

agocke Feb 8, 2018

nguerrera Feb 8, 2018

agocke Feb 8, 2018

jaredpar Feb 9, 2018

agocke Feb 8, 2018

agocke Feb 8, 2018

jaredpar Feb 8, 2018

jaredpar commented Feb 8, 2018

nguerrera commented Feb 8, 2018

jaredpar commented Mar 1, 2018

jaredpar commented Mar 1, 2018

agocke Mar 2, 2018

jaredpar Mar 5, 2018

agocke Mar 5, 2018

jaredpar Mar 5, 2018

agocke Mar 5, 2018

jaredpar Mar 5, 2018

jaredpar Mar 5, 2018

jaredpar Mar 5, 2018

agocke Mar 5, 2018 •

edited

Loading

jaredpar Mar 5, 2018

Multitarget our msbuild task #24672

Multitarget our msbuild task #24672

Conversation

jaredpar commented Feb 6, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

jaredpar commented Feb 8, 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

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

jaredpar commented Feb 8, 2018

nguerrera commented Feb 8, 2018

jaredpar commented Mar 1, 2018

jaredpar commented Mar 1, 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

Choose a reason for hiding this comment

agocke Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke Mar 5, 2018 •

edited

Loading