Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move ref assembly to the obj folder #6560

Merged
merged 9 commits into from
Jul 20, 2021
Merged

Move ref assembly to the obj folder #6560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9591bca
Move ref assembly to the obj folder
rainersigwald Jun 14, 2021
9e33e1d
Move the compiler output so it doesn't conflict with the final bin lo…
rainersigwald Jun 14, 2021
c06c78e
Create ref-assembly target directory
rainersigwald Jun 14, 2021
4577bd2
what if I close a paren? I mean, just, theoretically
rainersigwald Jun 15, 2021
4d1a2bd
Update the old way
rainersigwald Jun 15, 2021
ff27236
Merge remote-tracking branch 'upstream/main' into hide-ref-assemblies
rainersigwald Jul 12, 2021
71bee26
Changewave doc
rainersigwald Jul 12, 2021
6e18cd0
Create correct ref output directory in both cases
rainersigwald Jul 13, 2021
29b0904
Rename to ProduceReferenceAssemblyInOutDir
rainersigwald Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions documentation/wiki/ChangeWaves.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ The opt-out comes in the form of setting the environment variable `MSBuildDisabl
- [Scheduler should honor BuildParameters.DisableInprocNode](https://github.com/dotnet/msbuild/pull/6400)
- [Don't compile globbing regexes on .NET Framework](https://github.com/dotnet/msbuild/pull/6632)
- [Default to transitively copying content items](https://github.com/dotnet/msbuild/pull/6622)
- [Reference assemblies are now no longer placed in the `bin` directory by default](https://github.com/dotnet/msbuild/pull/6560)

## Change Waves No Longer In Rotation
8 changes: 5 additions & 3 deletions src/Tasks/Microsoft.Common.CurrentVersion.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,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 ('$(ProduceReferenceAssemblyInOutDir)' == '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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this one point to refint?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two ref assemblies:

  1. The one that the compiler produces as an output. This one is written to any time the compiler runs.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<!-- 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>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was so close!


<!-- Example, C:\MyProjects\MyProject\ -->
<ProjectDir Condition=" '$(ProjectDir)' == '' ">$([MSBuild]::EnsureTrailingSlash($(MSBuildProjectDirectory)))</ProjectDir>
Expand Down Expand Up @@ -393,9 +394,10 @@ 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)' == ''" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use TargetRefPath?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<CreateDirectory Include="@(IntermediateRefAssembly->'%(RootDir)%(Directory)')" />
<CreateDirectory Include="$(OutDir)ref" />
<CreateDirectory Include="$(OutDir)ref" Condition=" '$(ProduceReferenceAssemblyInOutDir)' == 'true'" />
<CreateDirectory Include="$(IntermediateOutputPath)ref" Condition=" '$(ProduceReferenceAssemblyInOutDir)' != 'true'" />
</ItemGroup>

<ItemGroup Condition="'$(_DebugSymbolsProduced)' == 'true'">
Expand Down