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.

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

.NET projects produce a reference assembly by default #8571

Conversation

drewnoakes
Copy link
Member

In general we want to produce reference assemblies whenever possible, as they improve incremental build performance.

Making them work end-to-end requires support from other components, such as fast up-to-date checks in VS. While support was originally added for SDK-style projects, it was not present for legacy .csproj files. Accordingly, ProduceReferenceAssemblies was set true only when the target framework was net5.0 or later for C#/VB projects. Similarly, F# added support during net7.0, so a similar check was added for F# projects.

VS 17.5 ships support for reference assemblies in the legacy CSPROJ fast up-to-date check, so all C#/VB projects are supported. Similarly, F# has full support. Consequently, it is now safe to enable the production and consumption of reference assemblies across all .NET project types, including .NET Framework, .NET Standard, .NET Core and .NET.

This PR changes the default ProduceReferenceAssembly value to true for all .NET projects.

In general we want to produce reference assemblies whenever possible, as they improve incremental build performance.

Making them work end-to-end requires support from other components, such as fast up-to-date checks in VS. While support was originally added for SDK-style projects, it was not present for legacy `.csproj` files. Accordingly, `ProduceReferenceAssemblies` was set `true` only when the target framework was `net5.0` or later for C#/VB projects. Similarly, F# added support during `net7.0`, so a similar check was added for F# projects.

VS 17.5 ships support for reference assemblies in the legacy CSPROJ fast up-to-date check, so all C#/VB projects are supported. Similarly, F# has full support. Consequently, it is now safe to enable the production and consumption of reference assemblies across all .NET project types, including .NET Framework, .NET Standard, .NET Core and .NET.

This commit changes the default `ProduceReferenceAssembly` value to `true` for all .NET projects.
The removed text exists in other files where it makes sense. It seems to be here due to copy/paste followed by an incomplete edit.
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Thank you.

I cannt think of any associated risk - but I'd prefer someone more experienced to have chance for last call - @rainersigwald, @ladipro?

@rainersigwald
Copy link
Member

I would like to hold this for 17.6 preview 1 if there's no critical need for it now.

@drewnoakes
Copy link
Member Author

I would like to hold this for 17.6 preview 1 if there's no critical need for it now.

Makes sense to me. I believe we'll also have to update SDK tests in response to this change.

@rainersigwald rainersigwald added this to the VS 17.7 milestone Mar 28, 2023
@Forgind Forgind added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Apr 3, 2023
@Forgind
Copy link
Member

Forgind commented Apr 3, 2023

I would like to hold this for 17.6 preview 1 if there's no critical need for it now.

Makes sense to me. I believe we'll also have to update SDK tests in response to this change.

@drewnoakes, are you planning to update the SDK tests/already updated the SDK tests? If so, once we have builds working properly, we think this is ready to go in.

@drewnoakes
Copy link
Member Author

are you planning to update the SDK tests/already updated the SDK tests? If so, once we have builds working properly, we think this is ready to go in.

I'm trying these changes in the SDK repo to understand what tests needed to be updated. So far I found only It_produces_ref_assembly_for_all_frameworks and It_builds_the_project_successfully_with_only_reference_assembly_set that need updates, but the tests take a while to run, and I haven't finished my investigation yet.

@drewnoakes
Copy link
Member Author

A first draft of the SDK changes are at: dotnet/sdk#31598

@rainersigwald rainersigwald merged commit 6487653 into dotnet:main Apr 12, 2023
@MarkOsborneMS
Copy link

MarkOsborneMS commented Apr 18, 2023

This change breaks with cached builds (project cache). Specifically when the version of msbuild being used to generate the cache output is older than the one being used on the developer's desktop. We get a cache hit because the input hash matches but the project cache outputs are missing the reference assembly.

rainersigwald added a commit that referenced this pull request Apr 18, 2023
@drewnoakes drewnoakes deleted the dev/drnoakes/enable-reference-assembly-by-default branch April 18, 2023 22:26
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Apr 21, 2023

I believe this change broke the .NET 8 Android workload's build in dotnet/android#7979:

Build Results.zip

/Users/builder/azdo/_work/1/s/xamarin-android/bin/Release/dotnet/sdk/8.0.100-preview.4.23220.4/Microsoft.Common.CurrentVersion.targets(4706,5): error MSB4044: The "CopyRefAssembly" task was not given a value for the required parameter "SourcePath".

I'm sure I can workaround, but I thought I'd share the log above if it is useful to understand how it might impact customers.

@MarkOsborneMS
Copy link

MarkOsborneMS commented Apr 21, 2023 via email

@jonathanpeppers
Copy link
Member

@MarkOsborneMS we are using .NET 8, so I think we just need the latest dotnet/msbuild to flow to dotnet/installer now, thanks.

I see the revert now: ac0b9a2

@rainersigwald
Copy link
Member

I thought I'd share the log above if it is useful to understand how it might impact customers

Definitely appreciated, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants