Move ref assembly to the obj folder #6560

rainersigwald · 2021-06-14T20:46:01Z

Context

The location of this file is mostly irrelevant, and users find it
confusing when it's in the output folder.

Changes Made

Changed the compiler's obj output to a folder named refint and changed the
"final" output ref assembly to live in obj/ref (modulo other variables).

Did this under a double opt-out: it's in the 17.0 changewave, and there's a new property $(ProduceReferenceAssemblyInBin).

Notes

Naming on . . . everything is up for debate.

The location of this file is mostly irrelevant, and users find it confusing when it's in the output folder.

jaredpar · 2021-06-14T20:50:32Z

Changed the compiler's obj output to a folder named refint and changed the

Missing where refint is specified. Can you help me see that?

…cation (and the CopyRefAssembly task can work)

rainersigwald · 2021-06-14T20:54:04Z

Can you help me see that?

I have decided to do this using the advanced technique of "actually doing what I said I did instead of lying". Sorry!

benvillalobos · 2021-06-14T21:46:07Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -323,7 +323,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 '$(ProduceReferenceAssembly)' == 'true' ">$([MSBuild]::NormalizePath($(TargetDir), 'ref', $(TargetFileName)))</TargetRefPath>
+    <TargetRefPath Condition=" '$(TargetRefPath)' == '' and '$(ProduceReferenceAssembly)' == 'true' and ('$(ProduceReferenceAssemblyInBin)' == 'true' or '$([MSBuild]::AreFeaturesEnabled(17.0))' != 'true' ) ">$([MSBuild]::NormalizePath($(TargetDir), 'ref', $(TargetFileName)))</TargetRefPath>


Seems redundant to have this in a change wave and have an opt in for it. By design shouldn't it be one or the other?

I expect this to be controversial enough to have a permanent flag. The changewave is just there since we have a big lever for "do stuff the old way".

Why not ProduceReferenceAssemblyInOutDir or ProduceReferenceAssemblyInOutputPath?

benvillalobos · 2021-06-14T21:46:34Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -323,7 +323,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 '$(ProduceReferenceAssembly)' == 'true' ">$([MSBuild]::NormalizePath($(TargetDir), 'ref', $(TargetFileName)))</TargetRefPath>
+    <TargetRefPath Condition=" '$(TargetRefPath)' == '' and '$(ProduceReferenceAssembly)' == 'true' and ('$(ProduceReferenceAssemblyInBin)' == 'true' or '$([MSBuild]::AreFeaturesEnabled(17.0))' != 'true' ) ">$([MSBuild]::NormalizePath($(TargetDir), 'ref', $(TargetFileName)))</TargetRefPath>
+    <TargetRefPath Condition=" '$(TargetRefPath)' == '' and '$(ProduceReferenceAssembly)' == 'true' ">$([MSBuild]::NormalizePath($(IntermediateOutputPath), 'ref', $(TargetFileName)))</TargetRefPath>


Should this one point to refint?

There are two ref assemblies:

The one that the compiler produces as an output. This one is written to any time the compiler runs.

The one that other projects reference. This one is copied from the former but only if it has changed, allowing the better incremental build of referencing projects.

TargetRefPath is the path of the latter; IntermediateRefAssembly the former.

@sharwell @jaredpar

Shouldn't the compiler generate the ref assembly only, if it detects the change (i.e. when doing a 2nd build or the 3rd, etc…)?
Generating every-time incurs perf cost, doesn't it? Is it possible to input the files already generated by the compiler to determine determinism? 😏

pardon my ignorance!

Shouldn't the compiler generate the ref assembly only, if it detects the change (i.e. when doing a 2nd build or the 3rd, etc…)?

No because that violates the areas of concern. The compiler is not a build system, it's a compiler. It's job is to take source and produce binaries (or more often errors). Once invoked the compiler should always produce the desired outputs. MSBuild, and the compiler MSBuild task, has the responsibilities of the build system and that includes up to date checks and such.

The way this works at a high level is that the CoreCompile target has two outputs: the ref and the impl assembly. The core compiler itself will always produce the ref and impl assembly when invoked. The Compiler MSBuild task, which wraps the compiler, essentially looks at the ref assembly output and determines if it is equal to the previous ref assembly output (just compare the MVID). If not then it doesn't update the ref assembly output from the task. Hence the output of the CoreCompile target has two outputs (ref + impl) but one is up to date because it has the same timestamp (the ref assembly) and the other is not up to date (the impl assembly). This lets MSBuild effectively short circuit later evaluations that depend only on the ref assembly.

pardon my ignorance!

Not ignorance at all. This is a tricky area to understand if you're not deeply involved. And most likely @rainersigwald is going to correct a few details I got wrong 😄

And most likely @rainersigwald is going to correct a few details I got wrong 😄

I've only got one small nit . . .

The Compiler MSBuild task, which wraps the compiler, essentially looks at the ref assembly output and determines if it is equal to the previous ref assembly output (just compare the MVID).

This is actually done by a separate task that lives in Roslyn, CopyRefAssembly, which is called here:

msbuild/src/Tasks/Microsoft.Common.CurrentVersion.targets

Lines 4524 to 4534 in d150e93



<CopyRefAssembly

SourcePath="@(IntermediateRefAssembly)"

DestinationPath="$(TargetRefPath)"

Condition="'$(ProduceReferenceAssembly)' == 'true' and '$(CopyBuildOutputToOutputDirectory)' == 'true' and '$(SkipCopyBuildProduct)' != 'true'"

>

<Output TaskParameter="DestinationPath" ItemName="ReferenceAssembly"/>

<Output TaskParameter="DestinationPath" ItemName="FileWrites"/>

</CopyRefAssembly>

I was so close!

benvillalobos · 2021-06-14T21:48:40Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

@@ -392,7 +393,7 @@ Copyright (C) Microsoft Corporation. All rights reserved.
  </ItemGroup>

  <ItemGroup Condition="'$(ProduceReferenceAssembly)' == 'true'">
-    <IntermediateRefAssembly Include="$(IntermediateOutputPath)ref\$(TargetName)$(TargetExt)" Condition="'@(IntermediateRefAssembly)' == ''" />
+    <IntermediateRefAssembly Include="$(IntermediateOutputPath)refint\$(TargetName)$(TargetExt)" Condition="'@(IntermediateRefAssembly)' == ''" />


This should use TargetRefPath?

Shouldn't this one and the change below be under ProduceReferenceAssemblyInOutputPath opt-in?

Why do you think this should be conditional? Do you know of people who reach directly into obj\ref today?

We have a logic that does that. But that is not the only argument…

It would be best for users, to either preserve compat all the way (helps not to have problems down the road) or break it and document the change to agree on one way of doing things by default.

To get the older behavior, right now, I'd have to change atleast three variables. With my proposal, we'll only need to change single variable.

Going OOF soon, don't want to block on this

benvillalobos · 2021-06-21T18:40:40Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

    <CreateDirectory Include="@(IntermediateRefAssembly->'%(RootDir)%(Directory)')" />
-    <CreateDirectory Include="$(OutDir)ref" />
+    <CreateDirectory Include="$(IntermediateOutputPath)ref" />


I feel like $(IntermediateOutputPath)refint should be added to this item. Does adding it to IntermediateRefAssembly ensure the path is getting created elsewhere?

That's what the prior entry does:

@(IntermediateRefAssembly->'%(RootDir)%(Directory)')

Shouldn't this and the above change also be under the opt-in property?

benvillalobos · 2021-06-28T21:48:32Z

If we have time for another build, can you modify the list in https://github.com/dotnet/msbuild/blob/main/documentation/wiki/ChangeWaves.md to include this PR?

agocke

LGTM

AraHaan · 2021-07-01T10:20:59Z

tbh I think it is a non-issue (or rather an issue related to projects that are not classlib projects), I have classlib projects and I depend on this for packaging reference assembles up into the same package as the actual runtime assemblies so that way if they try to browse the source from the nuget reference they will only see public members and not private implementation details (unless they look at the code on github).

I did this for a reason because 1, I think it is too much to package a version for "reference", and then package a separate one for "runtime" when I can produce just 1 package for both and it still works like I intend it to. Other projects might also came to depend on this. Unless this just changes for projects targeting only the 6.0 TFM. But I think the changes should only be targeted towards non-classlibs so classlibs that depend on the behavior before this PR can not be broken much.

Nirmal4G · 2021-07-13T18:28:57Z

This achieves what I was talking about in the issue, completely preserving the old way of outputting the ref assembly!

  <PropertyGroup>
    <ProduceReferenceAssemblyInOutputPath Condition="'$(ProduceReferenceAssemblyInOutputPath)' == '' and $([MSBuild]::AreFeaturesEnabled('17.0'))">false</ProduceReferenceAssemblyInOutputPath>
    <ProduceReferenceAssemblyInOutputPath Condition="'$(ProduceReferenceAssemblyInOutputPath)' == ''">true</ProduceReferenceAssemblyInOutputPath>
  </PropertyGroup>

  <PropertyGroup>
    <TargetRefPath Condition="'$(TargetRefPath)' == '' and '$(ProduceReferenceAssembly)' == 'true' and '$(ProduceReferenceAssemblyInOutputPath)' == 'false'">$([MSBuild]::NormalizePath('$(IntermediateOutputPath)', 'ref', '$(TargetFileName)'))</TargetRefPath>
    <TargetRefPath Condition="'$(TargetRefPath)' == '' and '$(ProduceReferenceAssembly)' == 'true'">$([MSBuild]::NormalizePath('$(TargetDir)', 'ref', '$(TargetFileName)'))</TargetRefPath>
  </PropertyGroup>

  <ItemGroup Condition="'$(ProduceReferenceAssembly)' == 'true' and '$(ProduceReferenceAssemblyInOutputPath)' == 'true'">
    <IntermediateRefAssembly Include="$(IntermediateOutputPath)ref\$(TargetName)$(TargetExt)" Condition="'@(IntermediateRefAssembly)' == ''" />
    <CreateDirectory Include="@(IntermediateRefAssembly->'%(RootDir)%(Directory)')" />
    <!-- Shouldn't this be $(TargetDir) instead of $(OutDir) -->
    <CreateDirectory Include="$(OutDir)ref" />
  </ItemGroup>

  <ItemGroup Condition="'$(ProduceReferenceAssembly)' == 'true' and '$(ProduceReferenceAssemblyInOutputPath)' == 'false'">
    <IntermediateRefAssembly Include="$(IntermediateOutputPath)refint\$(TargetName)$(TargetExt)" Condition="'@(IntermediateRefAssembly)' == ''" />
    <CreateDirectory Include="@(IntermediateRefAssembly->'%(RootDir)%(Directory)')" />
    <CreateDirectory Include="$(IntermediateOutputPath)ref" />
  </ItemGroup>

This is closer to the customization we document. Co-Authored-By: Nirmal Guru <Nirmal4G@gmail.com>

AraHaan · 2021-07-13T21:54:47Z

I agree I think moving the ref assemblies to the obj folder should be opt-in only to preserve codebases that expect them in the bin folder.

KathleenDollard · 2021-07-14T22:53:16Z

I think we need to make this change, and make sure it's in our breaking change docs, along with the information for opting out of the new behavior.

For most people, most of the time, this directory makes more sense in obj

benvillalobos · 2021-07-14T23:15:07Z

preemptively linking to our change wave docs for info on how to opt out

Nirmal4G · 2021-07-15T00:38:32Z

@KathleenDollard

It's okay to make a change but how we do it either makes it better or worse. Also, this feature is not for most users. For those who use this, they'd want the final output either in bin or in a new folder outside of bin/obj (mostly preferred).

See #6543 (comment), #6560 (comment)

jaredpar · 2021-07-15T03:24:09Z

I agree I think moving the ref assemblies to the obj folder should be opt-in only to preserve codebases that expect them in the bin folder.

Why? The default today is to put the ref assemblies, which have zero impact on program execution, into bin which is where customers typically deploy from.

This is particularly confusing for the new user case. They should know, and need to know, nothing about reference assemblies to code in .NET. But we are forcing them to learn about it by putting it directly in the place where we tell them to look for their program / build output.

ref assemblies are essentially an implementation detail of the build system. We shouldn't be putting it in place where we have observable output.

Fixes #6543 ### Context #6560 made `TargetRefPath` relative by mistake, which broke build in Visual Studio. ### Changes Made Use `MSBuildProjectDirectory` to properly root the path. ### Testing Verified that VS build works with this fix. ### Notes ``` 0:010> !pe Exception object: 0000020fa4de3a90 Exception type: System.IO.DirectoryNotFound ExceptionMessage: Could not find a part of the path 'C:\WINDOWS\system32\obj\Debug\net6.0\ref\HelloWorld.dll'. 0:010> !clrstack OS Thread Id: 0x95c (10) Child SP IP Call Site 000000bc6023cf88 00007ffb12634ed9 [HelperMethodFrame: 000000bc6023cf88] 000000bc6023d070 00007ffb037dba49 System.IO.__Error.WinIOError(Int32, System.String) 000000bc6023d0c0 00007ffb0408d7e6 System.IO.File.InternalCopy(System.String, System.String, Boolean, Boolean) 000000bc6023d150 00007ffaa7303d70 Microsoft.CodeAnalysis.BuildTasks.CopyRefAssembly.Copy() 000000bc6023d1b0 00007ffaa7303bad Microsoft.CodeAnalysis.BuildTasks.CopyRefAssembly.Execute() ```

This reverts commit 6dba77a.

Move ref assembly to the obj folder

9591bca

The location of this file is mostly irrelevant, and users find it confusing when it's in the output folder.

Move the compiler output so it doesn't conflict with the final bin lo…

9e33e1d

…cation (and the CopyRefAssembly task can work)

jaredpar approved these changes Jun 14, 2021

View reviewed changes

benvillalobos added the changewave17.0 label Jun 14, 2021

benvillalobos previously requested changes Jun 14, 2021

View reviewed changes

rainersigwald added 3 commits June 14, 2021 17:40

Create ref-assembly target directory

c06c78e

what if I close a paren? I mean, just, theoretically

4577bd2

Update the old way

4d1a2bd

benvillalobos reviewed Jun 21, 2021

View reviewed changes

marcpopMSFT mentioned this pull request Jun 30, 2021

Please move ref assets to obj directory dotnet/sdk#18596

Closed

agocke approved these changes Jul 1, 2021

View reviewed changes

marcpopMSFT added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jul 9, 2021

rainersigwald added 2 commits July 12, 2021 09:39

Merge remote-tracking branch 'upstream/main' into hide-ref-assemblies

ff27236

Changewave doc

71bee26

rainersigwald and others added 2 commits July 13, 2021 16:06

Create correct ref output directory in both cases

6e18cd0

Rename to ProduceReferenceAssemblyInOutDir

29b0904

This is closer to the customization we document. Co-Authored-By: Nirmal Guru <Nirmal4G@gmail.com>

rainersigwald removed the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jul 14, 2021

benvillalobos approved these changes Jul 20, 2021

View reviewed changes

benvillalobos added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jul 20, 2021

ladipro merged commit 6dba77a into dotnet:main Jul 20, 2021

ladipro mentioned this pull request Jul 21, 2021

Absolutize ref assembly path #6695

Merged

rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Jul 29, 2021

Revert "Move ref assembly to the obj folder (dotnet#6560)"

cad7e7b

This reverts commit 6dba77a.

rainersigwald mentioned this pull request Jul 29, 2021

Revert move of reference assemblies into obj #6718

Merged

rainersigwald deleted the hide-ref-assemblies branch September 7, 2021 22:18

rainersigwald mentioned this pull request Sep 10, 2021

Process-wide caching of ToolsetConfigurationSection #6832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ref assembly to the obj folder #6560

Move ref assembly to the obj folder #6560

rainersigwald commented Jun 14, 2021

jaredpar commented Jun 14, 2021

rainersigwald commented Jun 14, 2021

benvillalobos Jun 14, 2021

rainersigwald Jun 15, 2021

Nirmal4G Jul 13, 2021

benvillalobos Jun 14, 2021

rainersigwald Jun 15, 2021

Nirmal4G Jul 13, 2021

jaredpar Jul 13, 2021

rainersigwald Jul 13, 2021

jaredpar Jul 13, 2021

benvillalobos Jun 14, 2021

rainersigwald Jun 15, 2021

Nirmal4G Jul 13, 2021

rainersigwald Jul 13, 2021

Nirmal4G Jul 14, 2021

benvillalobos Jun 21, 2021

rainersigwald Jun 21, 2021

Nirmal4G Jul 13, 2021

benvillalobos commented Jun 28, 2021

agocke left a comment

AraHaan commented Jul 1, 2021

Nirmal4G commented Jul 13, 2021

AraHaan commented Jul 13, 2021

KathleenDollard commented Jul 14, 2021

benvillalobos commented Jul 14, 2021

Nirmal4G commented Jul 15, 2021

jaredpar commented Jul 15, 2021 •

edited

Loading

	<!-- Copy the reference assembly build product (.dll or .exe). -->
	<CopyRefAssembly
	SourcePath="@(IntermediateRefAssembly)"
	DestinationPath="$(TargetRefPath)"
	Condition="'$(ProduceReferenceAssembly)' == 'true' and '$(CopyBuildOutputToOutputDirectory)' == 'true' and '$(SkipCopyBuildProduct)' != 'true'"
	>

	<Output TaskParameter="DestinationPath" ItemName="ReferenceAssembly"/>
	<Output TaskParameter="DestinationPath" ItemName="FileWrites"/>

	</CopyRefAssembly>

Move ref assembly to the obj folder #6560

Move ref assembly to the obj folder #6560

Conversation

rainersigwald commented Jun 14, 2021

Context

Changes Made

Notes

jaredpar commented Jun 14, 2021

rainersigwald commented Jun 14, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvillalobos commented Jun 28, 2021

agocke left a comment

Choose a reason for hiding this comment

AraHaan commented Jul 1, 2021

Nirmal4G commented Jul 13, 2021

AraHaan commented Jul 13, 2021

KathleenDollard commented Jul 14, 2021

benvillalobos commented Jul 14, 2021

Nirmal4G commented Jul 15, 2021

jaredpar commented Jul 15, 2021 • edited Loading

jaredpar commented Jul 15, 2021 •

edited

Loading