Use refasm-only items for compilation #19146

rainersigwald · 2017-05-01T16:53:25Z

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.

rainersigwald · 2017-05-01T18:50:34Z

@jcouv: implemented as discussed in dotnet/msbuild#1986 (comment).

jcouv

Done with review pass. Thanks!

jcouv · 2017-05-01T18:53:56Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

  <Target Name="CoreCompile"
          Inputs="$(MSBuildAllProjects);
                  @(Compile);
                  @(_CoreCompileResourceInputs);
                  $(ApplicationIcon);
                  $(AssemblyOriginatorKeyFile);
-                  @(ReferencePath);
+                  @(ReferencePathWithInterfaceOnlyAssemblies);


Consider "ReferencePathWithRefAssemblies".

Also, can you clarify the benefit of introducing a new name (why not keep "ReferencePath", but feed annotated content into it)?

Also, can you clarify the benefit of introducing a new name (why not keep "ReferencePath", but feed annotated content into it)?

Never mind, I see you already explained the options in dotnet/msbuild#1986 (comment) Thanks!

jcouv · 2017-05-01T18:55:47Z

src/Compilers/Core/MSBuildTask/Microsoft.VisualBasic.Core.targets

+  <Target Name="ShimReferencePathsWhenCommonTargetsDoesNotUnderstandReferenceAssemblies"
+          BeforeTargets="CoreCompile"
+          Condition="'@(ReferencePathWithInterfaceOnlyAssemblies)' == ''">
+    <!-- Common targets should populate this item from 15.3, but this file


Nit: "dev15.3"

jcouv · 2017-05-01T18:57:18Z

@jaredpar @agocke @jasonmalinowski for review.

jasonmalinowski · 2017-05-01T19:27:43Z

I wasn't involved in the conversations of the ultimate decision to move to a new item type. @rainersigwald are the motivations there documented and written up somewhere? The MSBuild item I see has some discussions but didn't quite give the backstory. I am very worried about project system and extensibility impact, and I want to make sure those were thought about.

jcouv · 2017-05-01T19:37:23Z

@jasonmalinowski I think dotnet/msbuild#1986 (comment) will answer your questions.

rainersigwald · 2017-05-01T19:38:48Z

That was my reasoning. It's entirely possible I've missed something important, though! What project-system/extensibility impact are you worried about?

jasonmalinowski · 2017-05-01T19:54:09Z

There are two impacts I'm concerned about:

Other targets that might exist in the world that might be generating ReferencePath items that are listed as "run before CoreCompile" -- the PR here has no protection against that. The 'shim' target might run before such another target, and then those are broken. I'm not aware of stuff in the wild that does it, but I just wanted to raise it.
What places exist that consume ReferencePath? I know there's this in Roslyn and I see six references in the legacy project system. I don't know what exists in the new project system. Do we know if those need to be updated as well?

To be clear: I don't think this is the wrong approach...I just don't (personally) know if it's the right approach either. 😄

jcouv · 2017-05-02T22:04:41Z

build/scripts/build.ps1

@@ -25,7 +25,7 @@ function Print-Usage() {
 }

 function Run-Build() {
-    $buildArgs = "/v:m /m"


Is this intended?

jcouv · 2017-05-02T22:05:14Z

src/Compilers/Core/Portable/AdditionalTextFile.cs

@@ -64,3 +64,5 @@ public override SourceText GetText(CancellationToken cancellationToken = default
        internal IList<DiagnosticInfo> Diagnostics => _diagnostics;
    }
 }
+


Please revert this file and the one below (empty changes). Thanks

jcouv · 2017-05-02T22:07:00Z

@dotnet/roslyn-compiler for review. I will try to produce a new signed build with this for end-to-end testing in VS tomorrow.

@jasonmalinowski Is there anyone else that we should pull in to review integration with project system?

jaredpar · 2017-05-02T22:11:28Z

build/scripts/build.ps1

@@ -25,7 +25,7 @@ function Print-Usage() {
 }

 function Run-Build() {
-    $buildArgs = "/v:m /m"
+    $buildArgs = "/v:m /m /flp:v=diag"


This slows down our build considerably. Why do we need to make this a part of the dev workflow?

You don't, local testing change escaped when I git commit -am-ed. Will revert.

agocke · 2017-05-02T22:33:44Z

I'm concerned about lack of tests here. How are we supposed to verify this change?

jasonmalinowski · 2017-05-03T00:35:54Z

@dotnet/project-system can help review the project system impacts, as well as @dotnet/roslyn-ide who might have some knowledge of the legacy project system impacts.

jcouv · 2017-05-03T03:01:53Z

@agocke Yes, I ran into this problem in my own recent change for targets file :-( I did local verification then.
Maybe @rainersigwald would have some suggestions on validation strategy for targets files, since that seems a common msbuild problem.

That said, we're planning to produce a VS build with refout bits from msbuild and Roslyn, this week, so that we can do end-to-end validation. That will help cover.

jcouv

LGTM

tmeschter · 2017-05-03T18:06:18Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

+         may be used (via NuGet package) on earlier MSBuilds. If the
+         adjusted-for-reference-assemblies item is not populated, just use
+         the older item's contents. -->
+    <ItemGroup>


If you're using a newer MSBuild is there any legitimate reason that ReferencePathWithRefAssemblies would be empty? If so are we breaking anything by forcing a value?

The new common targets from dotnet/msbuild#2039 unconditionally populate @(ReferencePathWithRefAssemblies) here, so no--it shouldn't be empty unless @(ReferencePath) was too, in which case this should be harmless.

rainersigwald · 2017-05-03T18:21:09Z

@agocke Unfortunately, testing this kind of change in isolation is very difficult. MSBuild still runs a fairly large suite of (internal) tests that run builds of projects end-to-end and test for certain behaviors, and we should augment that test suite with some specific tests for ref-asm behavior. Does Roslyn have a similar suite, or are y'all dependent on others to catch build flow regressions? RPS scenarios also cover many build end-to-end configurations.

One major problem with the existing tests is that they work in VS-land, which requires having insertions of outside-the-VS-repo code. In order to run those tests, this change needs to be available in a Roslyn VSIX, so that it can be combined with an MSBuild VSIX built from the feature branch on our side and tested end-to-end. In MSBuild, we don't have a strict review policy on creating such a feature branch, getting a build from it, and testing it in VS--anyone on the core team can do that at their discretion. I don't know what the policies are in this repo.

This PR is what I'd like to see tested in that way--I would not be surprised if that testing revealed problems which will require followup, either here or in MSBuild. But I think the best way to gain confidence in this approach is to test it in VS.

jcouv · 2017-05-03T19:34:55Z

Reviewers, any other feedback/questions/blockers?

jasonmalinowski · 2017-05-03T19:37:03Z

@jcouv Do we have the test plan written up somewhere?

jcouv · 2017-05-03T19:45:00Z

@jasonmalinowski Short answer is no, but I can work on that.
We have a placeholder test plan for compiler changes (#18612). Feel free to list suggestions there.
I have not thought systematically about end-to-end validation. We were just going to dogfood VS on Roslyn and MSBuild and also self-host (update our toolset).

Is there anything specific you'd like to see?

jasonmalinowski · 2017-05-03T19:52:51Z

@jcouv I'll add comments over there.

agocke · 2017-05-03T19:57:24Z

@rainersigwald @jcouv Is there any way we could get this into our bootstrap build before merging? Say, if we produced the MSBuild bits first and then updated Roslyn to consume them, then verified in the bootstrap build that we saw the expected behavior?

agocke · 2017-05-03T20:00:02Z

@jaredpar IIRC you added some validation to our bootstrap build to a build task before. Is that a viable option now?

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).

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.

agocke

Talked with @jcouv. I'm convinced there's nothing we can do right now to improve validation within Roslyn. Once more bits are merged into master we can re-evaluate

jcouv · 2017-05-04T21:36:04Z

Relates to #18612

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

jcouv reviewed May 1, 2017

View reviewed changes

jcouv added the Area-Compilers label May 1, 2017

jcouv added this to the 15.3 milestone May 1, 2017

jcouv reviewed May 2, 2017

View reviewed changes

jaredpar reviewed May 2, 2017

View reviewed changes

rainersigwald force-pushed the refout-msbuild-infrastructure branch from ed0945c to ae43f4d Compare May 2, 2017 22:46

jasonmalinowski requested review from jasonmalinowski, Pilchie and tmeschter May 3, 2017 00:36

jcouv approved these changes May 3, 2017

View reviewed changes

tmeschter reviewed May 3, 2017

View reviewed changes

jasonmalinowski approved these changes May 3, 2017

View reviewed changes

rainersigwald added 2 commits May 3, 2017 15:23

Use CopyRefAssembly from NuGet package

ae50fe8

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.

rainersigwald force-pushed the refout-msbuild-infrastructure branch from ae43f4d to ae50fe8 Compare May 3, 2017 20:24

agocke approved these changes May 3, 2017

View reviewed changes

jcouv merged commit a255d49 into dotnet:features/refout May 3, 2017

Adam-S-Daniel mentioned this pull request Sep 6, 2018

Support TargetsTriggeredBeforeCompilation property #24053

Closed

rainersigwald deleted the refout-msbuild-infrastructure branch May 18, 2019 02:37

This was referenced Nov 6, 2022

ReferencePathWithRefAssemblies missing for CoreCompile during DesignTime builds in Ionide.ProjInfo dotnet/fsharp#14250

Open

getProjectInfo doesn't return framework references? ionide/proj-info#171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use refasm-only items for compilation #19146

Use refasm-only items for compilation #19146

rainersigwald commented May 1, 2017

rainersigwald commented May 1, 2017

jcouv left a comment

jcouv May 1, 2017

jcouv May 1, 2017

jcouv May 1, 2017

jcouv May 1, 2017

jcouv commented May 1, 2017

jasonmalinowski commented May 1, 2017

jcouv commented May 1, 2017

rainersigwald commented May 1, 2017

jasonmalinowski commented May 1, 2017

jcouv May 2, 2017

jcouv May 2, 2017

jcouv commented May 2, 2017

jaredpar May 2, 2017

rainersigwald May 2, 2017

agocke commented May 2, 2017

jasonmalinowski commented May 3, 2017

jcouv commented May 3, 2017

jcouv left a comment

tmeschter May 3, 2017

rainersigwald May 3, 2017

rainersigwald commented May 3, 2017

jcouv commented May 3, 2017

jasonmalinowski commented May 3, 2017

jcouv commented May 3, 2017

jasonmalinowski commented May 3, 2017

agocke commented May 3, 2017

agocke commented May 3, 2017

agocke left a comment

jcouv commented May 4, 2017

Use refasm-only items for compilation #19146

Use refasm-only items for compilation #19146

Conversation

rainersigwald commented May 1, 2017

rainersigwald commented May 1, 2017

jcouv 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

jcouv commented May 1, 2017

jasonmalinowski commented May 1, 2017

jcouv commented May 1, 2017

rainersigwald commented May 1, 2017

jasonmalinowski commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented May 2, 2017

jasonmalinowski commented May 3, 2017

jcouv commented May 3, 2017

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald commented May 3, 2017

jcouv commented May 3, 2017

jasonmalinowski commented May 3, 2017

jcouv commented May 3, 2017

jasonmalinowski commented May 3, 2017

agocke commented May 3, 2017

agocke commented May 3, 2017

agocke left a comment

Choose a reason for hiding this comment

jcouv commented May 4, 2017