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

[davkean]

Version 15.3.0-pre.1.0+26524.3003.d15prerel

This target which we copied from a package that comes in the VS SDK is broken in this build:

  <!-- This is a copy of the Microsoft.VisualStudio.SDK.EmbedInteropTypes NuGet package, but only the list of
       assemblies that we need. The package includes things like EnvDTE which are reasonable for consumers, but
       strange since we actually _implement_ DTE and use it as an exchange type with generics in a few places. -->
  <Target Name="LinkVSSDKEmbeddableAssemblies" DependsOnTargets="ResolveReferences" AfterTargets="ResolveReferences">
    <ItemGroup>
      <ReferencePath Condition="
              '%(FileName)' == 'Microsoft.VisualStudio.Shell.Embeddable'
           or '%(FileName)' == 'Microsoft.VisualStudio.Shell.Interop.12.0'
           or '%(FileName)' == 'Microsoft.VisualStudio.Shell.Interop.12.1.DesignTime'
           or '%(FileName)' == 'Microsoft.VisualStudio.Shell.Interop.14.0.DesignTime'
           or '%(FileName)' == 'Microsoft.VisualStudio.Shell.Interop.15.0.DesignTime'
           or '%(Filename)' == 'Microsoft.VisualStudio.Imaging.Interop.14.0.DesignTime'
           or '%(FileName)' == 'Microsoft.VisualStudio.TextManager.Interop.12.1.DesignTime'
           or '%(FileName)' == 'Microsoft.VisualStudio.ProjectSystem.Interop'
           or '%(FileName)' == 'stdole'
           or '%(FileName)' == 'Microsoft.VisualStudio.CommandBars'
           or '%(FileName)' == 'NuGet.SolutionRestoreManager.Interop'
           or '%(FileName)' == 'NuGet.VisualStudio'
              ">
        <EmbedInteropTypes>true</EmbedInteropTypes>
      </ReferencePath>
    </ItemGroup>
  </Target>

ie Changes this target makes to the ReferencePath item are not respected at compile time.

This silently breaks the product until runtime, after building with this binary - it fails to load because Visual Studio doesn't ship with any of the .DesignTime dlls, hence why they are embedded. Manipulating ReferencePath is a pretty common extension point.

I'm guessing this was broken in the references assembly changes - haven't fully groked what changes we made there and where we landed with all this.

[davkean]

tag @rainersigwald @AndyGerlicher

[rainersigwald]

This is indeed a consequence of #2039. I'm not sure what to do about it.

This target is arguably wrong--it's manipulating an item after the rollup target that logically produces that item. I'd rather see it as BeforeTargets="ResolveReferences" DependsOnTargets="$(ResolveReferencesDependsOn)", similar to what I did with the find-ref-assemblies target. Though ordering between two different targets that want to do related things is extremely tricky there.

Being "wrong" in a non-obvious way doesn't mean we can just break things with impunity. But we also need to be able to modify the build process.

Thinking about fixes.

@davkean You can work around for the moment by adding a BeforeTargets="FindReferenceAssembliesForReferences" to that target. EDIT: wait, that probably creates a dependency cycle. You probably have to replace the dependencies with that rather than add it.

[rainersigwald]

One possibility would be to move FindReferenceAssembliesForReferences to be BeforeTargets="BeforeCompile" (or _GenerateCompileInputs?). Or to add it as an explicit named, ordered dependency in $(CompileDependsOn).

Would that make it happen too late for some uses? I can't think of any offhand, since @(ReferencePathWithRefAssemblies) is used only in CoreCompile at the moment--and it's new, so if someone wanted to take a dependency on it they'd have to deal with it wherever it is.

cc @jcouv

[rainersigwald]

Confirmed by patching my 15.3.0-pre.1.0+26524.0.d15rel install that what I'm proposing in #2139 fixes the issue for the project system repo.

[nguerrera]

Naive question: why not just have compilation tasks use @(ReferencePath->'%(ReferenceAssembly)') and eliminate the separate item group?

[rainersigwald]

@nguerrera That's discussed in #1986 (comment), but the short version is "target incrementality" + "backward compat". In retrospect, it could have been done that way by applying the transform everywhere (including in the inputs) and making the compat shim in Roslyn populate that metadata on the item.