Common.targets support for reference assemblies #2039

rainersigwald · 2017-05-01T16:31:50Z

Provides an item, ReferencePathWithInterfaceOnlyAssemblies, that
consists of the reference (interface-only) versions of assemblies where
available, allowing a consuming task to be incrementally up-to-date
after implementation-only changes have been made to a reference.

When %(ReferenceAssembly) metadata is unavailable for a given
reference, the new item contains the implementation assembly directly.

Additionally creates a new output item for the current project's
reference assembly by default when $(Deterministic) is true. If this
is created, it is copied to the output directory using the new
CopyRefAssembly task, which copies only if the ref assembly is
different from the current file. If present, the ref asm is passed to
the project's consumes in metadata.

See #1986, dotnet/roslyn#2184.

Provides an item, `ReferencePathWithInterfaceOnlyAssemblies`, that consists of the reference (interface-only) versions of assemblies where available, allowing a consuming task to be incrementally up-to-date after implementation-only changes have been made to a reference. When `%(ReferenceAssembly)` metadata is unavailable for a given reference, the new item contains the implementation assembly directly. Additionally creates a new output item for the current project's reference assembly by default when `$(Deterministic)` is `true`. If this is created, it is copied to the output directory using the new `CopyRefAssembly` task, which copies only if the ref assembly is different from the current file. If present, the ref asm is passed to the project's consumes in metadata. See #1986, dotnet/roslyn#2184.

rainersigwald

Prelim self-review.

rainersigwald · 2017-05-01T16:32:55Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -350,6 +352,13 @@ Copyright (C) Microsoft Corporation. All rights reserved.
    <FinalDocFile Include="@(DocFileItem->'$(OutDir)%(Filename)%(Extension)')"/>
  </ItemGroup>

+  <ItemGroup Condition="'$(Deterministic)' == 'true'">


Needs an explicit "no ref asm" off switch, as well as defaulting for Deterministic.

Condition must also apply to $(TargetRefPath).

rainersigwald · 2017-05-01T16:33:40Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -350,6 +352,13 @@ Copyright (C) Microsoft Corporation. All rights reserved.
    <FinalDocFile Include="@(DocFileItem->'$(OutDir)%(Filename)%(Extension)')"/>
  </ItemGroup>

+  <ItemGroup Condition="'$(Deterministic)' == 'true'">
+    <!-- TODO: should this be configurable? Default path obeys conventions. -->


I'm thinking "no", let the user define the item themselves if they want a nonstandard path.

rainersigwald · 2017-05-01T16:34:52Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

  </Target>

+  <ItemDefinitionGroup>
+    <ReferencePath>
+      <ReferenceAssembly>%(FullPath)</ReferenceAssembly>


Comment this.

rainersigwald · 2017-05-01T16:35:11Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+        Condition="'$(CopyBuildOutputToOutputDirectory)' == 'true' and '$(SkipCopyBuildProduct)' != 'true' and Exists('@(IntermediateRefAssembly)')"
+            >
+
+      <Output TaskParameter="DestinationPath" ItemName="ReferenceAssembly"/>


Add output to the outputs-of-this-project item so clean works right.

jeffkl

I'm concerned about how this will work with Clean, have you tested it?

jeffkl · 2017-05-01T16:36:04Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -281,6 +281,8 @@ Copyright (C) Microsoft Corporation. All rights reserved.
    <!-- Example, c:\MyProjects\MyProject\bin\debug\MyAssembly.dll -->
    <TargetPath Condition=" '$(TargetPath)' == '' ">$(TargetDir)$(TargetFileName)</TargetPath>

+    <TargetRefPath Condition=" '$(TargetRefPath)' == '' and '$(Deterministic)' == 'true' ">$([System.IO.Path]::Combine(`$([System.IO.Path]::GetDirectoryName($([System.IO.Path]::GetFullPath(`$(TargetPath)`))))`, 'ref', `$(TargetFileName)`))</TargetRefPath>


Can this just use $(TargetDir)?

Then you could use $([MSBuild]::NormalizePath($(TargetDir), 'ref', $(TargetFileName)))

That was my original plan, but it would break the case where TargetPath is explicitly specified, which seems to be legal from my reading of the prior set of conditions.

I'd be fairly ok with saying "if you want to do that odd thing, you must also specify this" but it needs to be an explicit choice.

I think if a user wants to set $(TargetPath) they should also have to set $(TargetRefPath) right? There's no guarantee that appending ref\$(TargetFileName) to whatever they specified as TargetPath would make sense. I'll let you decide...

jeffkl · 2017-05-01T16:38:16Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -1797,6 +1806,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
      <TargetPathWithTargetPlatformMoniker Include="$(TargetPath)">
        <TargetPlatformMoniker>$(TargetPlatformMoniker)</TargetPlatformMoniker>
        <TargetPlatformIdentifier>$(TargetPlatformIdentifier)</TargetPlatformIdentifier>
+        <ReferenceAssembly Condition="'$(TargetRefPath)' != ''">$(TargetRefPath)</ReferenceAssembly>


Does this need to be conditioned? It seemed unusual to me, most other places just copy the empty value.

This is the only field that's optional, right? If this particular project doesn't have a ref asm for whatever reason, we don't want to populate this field because it'll break downstream projects.

I think this'll be clearer when I just use a big off switch for the entire feature, rather than checking whether that path is populated. Seem reasonable?

From what I've seen, setting metadata is never conditioned because the getting of the value is checked later. But if you like this pattern more I'll let you do what you think is best.

jeffkl · 2017-05-01T16:39:21Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+        Condition="'$(CopyBuildOutputToOutputDirectory)' == 'true' and '$(SkipCopyBuildProduct)' != 'true' and Exists('@(IntermediateRefAssembly)')"
+            >
+
+      <Output TaskParameter="DestinationPath" ItemName="ReferenceAssembly"/>


Will this be in FileWrites? It probably needs to be in this item group almost always otherwise it will get deleted in between builds right?

Confirmed that after my last changes Clean now deletes the ref assembly from both bin and obj, and IncrementalClean leaves it alone. Is there another scenario to test?

jcouv · 2017-05-01T20:55:44Z

FYI @jasonmalinowski (this is the core change for MSBuild to handle ref assemblies).

cdmihai · 2017-05-02T19:27:39Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -154,6 +154,9 @@ Copyright (C) Microsoft Corporation. All rights reserved.
    <_DocumentationFileProduced>true</_DocumentationFileProduced>
    <_DocumentationFileProduced Condition="'$(DocumentationFile)'==''">false</_DocumentationFileProduced>

+    <!-- Whether or not a reference assembly is produced-->
+    <ProduceReferenceAssembly>false</ProduceReferenceAssembly>


Are users supposed to use this property to turn ref assemblies off?

To not create them in this project. They'll still be consumed if produced in other projects.

Better names welcome.

cdmihai · 2017-05-02T19:42:40Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -2015,8 +2028,21 @@ Copyright (C) Microsoft Corporation. All rights reserved.
      <Output TaskParameter="FilesWritten" ItemName="FileWrites"/>
      <Output TaskParameter="DependsOnSystemRuntime" PropertyName="DependsOnSystemRuntime"/>
    </ResolveAssemblyReference>
+
+    <ItemGroup>
+      <ReferencePathWithInterfaceOnlyAssemblies Include="@(ReferencePath->'%(ReferenceAssembly)')" />


Is it OK to have empty strings for the ReferencePath items that do not have ref assemblies?

Ah, the item def group adding a ref assembly value for each item is in the next diff, ignore :)

In reply to: 114407016 [](ancestors = 114407016)

…blies

rainersigwald · 2017-05-02T21:49:09Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -154,6 +154,9 @@ Copyright (C) Microsoft Corporation. All rights reserved.
    <_DocumentationFileProduced>true</_DocumentationFileProduced>
    <_DocumentationFileProduced Condition="'$(DocumentationFile)'==''">false</_DocumentationFileProduced>

+    <!-- Whether or not a reference assembly is produced-->
+    <ProduceReferenceAssembly>false</ProduceReferenceAssembly>


if not already defined!

AndyGerlicher · 2017-05-02T23:01:36Z

@dotnet-bot test this please

rainersigwald · 2017-05-03T15:15:25Z

@dotnet-bot test this please

(New CI jobs might be borked)

jasonmalinowski · 2017-05-03T20:08:04Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -1797,6 +1809,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
      <TargetPathWithTargetPlatformMoniker Include="$(TargetPath)">
        <TargetPlatformMoniker>$(TargetPlatformMoniker)</TargetPlatformMoniker>
        <TargetPlatformIdentifier>$(TargetPlatformIdentifier)</TargetPlatformIdentifier>
+        <ReferenceAssembly Condition="'$(ProduceReferenceAssembly)' != ''">$(TargetRefPath)</ReferenceAssembly>


Should this condition be if ProduceReferenceAssembly == true? Otherwise if false we'll still be setting this.

jasonmalinowski · 2017-05-03T20:08:38Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+         if this metadatum is always populated. This item definition ensures that it points
+         to the implementation assembly unless specified. -->
+    <ReferencePath>
+      <ReferenceAssembly>%(FullPath)</ReferenceAssembly>


That is magic. ✨

Alas, it was too magic: #2165

jasonmalinowski · 2017-05-03T20:13:43Z

src/Tasks/Microsoft.Common.tasks

@@ -172,5 +172,6 @@
    <!-- Roslyn tasks are now in an assembly owned and shipped by Roslyn -->
    <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Csc"                       AssemblyFile="$(RoslynTargetsPath)\Microsoft.Build.Tasks.CodeAnalysis.dll" Condition="'$(MSBuildAssemblyVersion)' != ''" />
    <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.Vbc"                       AssemblyFile="$(RoslynTargetsPath)\Microsoft.Build.Tasks.CodeAnalysis.dll" Condition="'$(MSBuildAssemblyVersion)' != ''" />
+    <UsingTask TaskName="Microsoft.CodeAnalysis.BuildTasks.CopyRefAssembly"           AssemblyFile="$(RoslynTargetsPath)\Microsoft.Build.Tasks.CodeAnalysis.dll" Condition="'$(MSBuildAssemblyVersion)' != ''" />


What's the expected behavior if the user has a newer version of the Roslyn package installed? I believe that package does UsingTasks on Csc/Vbc so those get overridden. Should we be doing the UsingTask too there so that way an upgrade also means you get a newer version here? (cc @jcouv)

Excellent catch! I added it to the Roslyn package props in dotnet/roslyn@ae50fe8

Thanks 😄

Use the new item provided by common targets to avoid recompiling a project when a reference has changed only in implementation, not interface. Include a shim so that these targets can continue to work on older MSBuild distributions. That keeps the NuGet-package-for-downlevel scenario working, as well as making an easy transition period to the new logic. Requires dotnet/msbuild#2039 to enable end-to-end use of reference assemblies, but is compatible with earlier MSBuilds (without using reference assemblies).

…get, and shim (#19146) * Use refasm-only items for compilation Use the new item provided by common targets to avoid recompiling a project when a reference has changed only in implementation, not interface. Include a shim so that these targets can continue to work on older MSBuild distributions. That keeps the NuGet-package-for-downlevel scenario working, as well as making an easy transition period to the new logic. Requires dotnet/msbuild#2039 to enable end-to-end use of reference assemblies, but is compatible with earlier MSBuilds (without using reference assemblies). * Use CopyRefAssembly from NuGet package The Microsoft.Net.Compilers package delivers UsingTasks to override the "standard" ones provided by MSBuild's Microsoft.Common.tasks. That should include the new CopyRefAssembly.

jcouv · 2017-05-04T21:34:44Z

Relates to dotnet/roslyn#18612

This should avoid problems with missing references caused by ImplicitlyExpandDesignTimeFacades running after RAR (and therefore after the creation of ReferencePathWithRefAssemblies prior to this commit).

Introduce $(CompileUsingReferenceAssemblies), which can avoid passing reference assemblies to the compiler when set to false.

IDE fast up-to-date checks need to be able to compare timestamps on _both_ the reference assembly (to see if a recompile is needed) and the implementation assembly (because it needs to be copied to an output location). Provide metadata on the adjusted references that points back to the implementation assembly if a ref assembly was found.

Since there's a single clear off switch, depend on it instead of various Exists checks (avoiding I/O) and instead of $(Deterministic).

And decouple it from $(Deterministic) entirely.

When a project both uses and produces reference assemblies, its primary output assembly might be up to date while (transitive) references need to be copied to its output directory. This case fooled the Visual Studio Fast Up-To-Date check, because the transitive dependencies aren't listed as project inputs (because design-time builds disable RAR's FindDependencies for performance reasons). To account for this, introduce a new file that is updated whenever _CopyFilesMarkedCopyLocal does actual work, pass it along as part of the project's output, and consider it a project input for the FUTD check.

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

rainersigwald commented May 1, 2017

View reviewed changes

jeffkl requested changes May 1, 2017

View reviewed changes

rainersigwald mentioned this pull request May 1, 2017

Use refasm-only items for compilation dotnet/roslyn#19146

Merged

rainersigwald added 3 commits May 1, 2017 11:59

Comment itemdefinitiongroup

5c8367f

Fix comment on CopyRefAssembly

f36d4dd

Add reference assembly to @(FileWrites)

ee73827

rainersigwald mentioned this pull request May 1, 2017

Support reference assemblies in build flows #1986

Closed

12 tasks

Add explicit ProduceReferenceAssembly switch

f25de78

cdmihai reviewed May 2, 2017

View reviewed changes

ReferencePathWithInterfaceOnlyAssemblies -> ReferencePathWithRefAssem…

e524382

…blies

rainersigwald commented May 2, 2017

View reviewed changes

rainersigwald added 2 commits May 3, 2017 10:32

Pay better attention to $(ProduceReferenceAssembly) defined in proj

c7e973c

Fix param name in CopyRefAssembly

6babd9f

jasonmalinowski approved these changes May 3, 2017

View reviewed changes

Return ReferenceAssembly only when explicitly enabled

70f1aaa

jeffkl approved these changes May 4, 2017

View reviewed changes

rainersigwald added 3 commits May 5, 2017 14:34

Create separate FindReferenceAssembliesForReferences

c7679e4

This should avoid problems with missing references caused by ImplicitlyExpandDesignTimeFacades running after RAR (and therefore after the creation of ReferencePathWithRefAssemblies prior to this commit).

Explicit off for consuming ref assemblies

535af86

Introduce $(CompileUsingReferenceAssemblies), which can avoid passing reference assemblies to the compiler when set to false.

rainersigwald force-pushed the feature/roslyn-refout branch from 197900d to f357dc1 Compare May 5, 2017 23:09

Add to IDE fast-up-to-date-check supplemental input

1856345

Include CopyLocalFiles in UpToDateCheckInput

e105500

rainersigwald mentioned this pull request May 12, 2017

Document new properties related to reference assemblies #2096

Closed

2 tasks

rainersigwald added 2 commits May 12, 2017 14:46

Depend directly on $(ProduceReferenceAssembly)

56ab56c

Since there's a single clear off switch, depend on it instead of various Exists checks (avoiding I/O) and instead of $(Deterministic).

Default ProduceReferenceAssembly to false

fc64b35

And decouple it from $(Deterministic) entirely.

AndyGerlicher approved these changes May 12, 2017

View reviewed changes

rainersigwald force-pushed the feature/roslyn-refout branch from 3aceaa8 to a5cf059 Compare May 19, 2017 21:19

rainersigwald merged commit e3a3d69 into master May 19, 2017

rainersigwald deleted the feature/roslyn-refout branch May 19, 2017 22:07

This was referenced May 22, 2017

Up-to-date check awareness of reference assemblies dotnet/project-system#2254

Closed

[Breaking Change] Targets that run after ResolveReferences to manipulate references are broken after upgrading to 15.3 #2134

Closed

rainersigwald mentioned this pull request Jun 7, 2017

Projects that override GetTargetPath have outputs ignored in referencing projects #2183

Closed

nguerrera mentioned this pull request Jun 7, 2018

Don't use ResolvedPath metadata on ReferencePath dotnet/linker#324

Closed

rainersigwald mentioned this pull request Feb 21, 2019

Use reference assemblies in the MSBuild repo #4195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common.targets support for reference assemblies #2039

Common.targets support for reference assemblies #2039

rainersigwald commented May 1, 2017

rainersigwald left a comment •

edited

Loading

rainersigwald May 1, 2017

rainersigwald May 1, 2017

rainersigwald May 1, 2017

rainersigwald May 1, 2017

rainersigwald May 1, 2017

jeffkl left a comment

jeffkl May 1, 2017

rainersigwald May 1, 2017

jeffkl May 1, 2017

jeffkl May 1, 2017

rainersigwald May 1, 2017

jeffkl May 1, 2017

jeffkl May 1, 2017

rainersigwald May 3, 2017

jcouv commented May 1, 2017

cdmihai May 2, 2017

rainersigwald May 2, 2017

cdmihai May 2, 2017 •

edited

Loading

cdmihai May 2, 2017 •

edited

Loading

rainersigwald May 2, 2017

AndyGerlicher commented May 2, 2017

rainersigwald commented May 3, 2017

jasonmalinowski May 3, 2017

rainersigwald May 3, 2017

jasonmalinowski May 3, 2017

rainersigwald Jun 5, 2017

jasonmalinowski May 3, 2017

rainersigwald May 3, 2017

jcouv May 3, 2017

jcouv commented May 4, 2017

Common.targets support for reference assemblies #2039

Common.targets support for reference assemblies #2039

Conversation

rainersigwald commented May 1, 2017

rainersigwald left a comment • 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

jeffkl left a 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

jcouv commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdmihai May 2, 2017 • edited Loading

Choose a reason for hiding this comment

cdmihai May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyGerlicher commented May 2, 2017

rainersigwald commented May 3, 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

jcouv commented May 4, 2017

rainersigwald left a comment •

edited

Loading

cdmihai May 2, 2017 •

edited

Loading

cdmihai May 2, 2017 •

edited

Loading