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

Default ProduceReferenceAssembly to true #2521

Closed
dasMulli opened this issue Sep 7, 2018 · 24 comments · Fixed by #12720
Closed

Default ProduceReferenceAssembly to true #2521

dasMulli opened this issue Sep 7, 2018 · 24 comments · Fixed by #12720
Assignees
Labels
Milestone

Comments

@dasMulli
Copy link
Contributor

dasMulli commented Sep 7, 2018

Get better incremental build performance by opting in to reference assembly based builds by defaulting ProduceReferenceAssembly to true.

Since this is a behaviour change, it may be good to do it in 3.0 and for SDK projects only.

@dasMulli
Copy link
Contributor Author

dasMulli commented Sep 7, 2018

@peterhuene I'm not sure who I should mention for this.

maybe @rainersigwald has more info about the current support for this across project systems, he mentioned a year ago that the classic project systems may have troubles with it, but that was when the feature was newly implemented.

@rainersigwald
Copy link
Member

@davkean were you looking into this?

@dasMulli
Copy link
Contributor Author

dasMulli commented Oct 1, 2018

@davkean ping 😄

@rainersigwald @dsplaisted @peterhuene @livarcocc @nguerrera any objections?

@rainersigwald
Copy link
Member

I'm still worried about the effect in VS on projects that have a reference to an SDK project but are themselves a non-SDK csproj-project-system project. Would love to find out that that's all been fixed but I don't think it has, yet.

@nguerrera nguerrera added this to the 3.0.1xx milestone Oct 1, 2018
@nguerrera
Copy link
Contributor

cc @Pilchie My understanding is that we intend for this to be the default in 3.0

I'm still worried about the effect in VS on projects that have a reference to an SDK project but are themselves a non-SDK csproj-project-system project

Are you worried about a mix of projects with ProduceReferenceAssembly=true/false irrespective of SDK/non-SDK? Shouldn't that be supported?

Would love to find out that that's all been fixed but I don't think it has

What is it that might not be fixed?

@rainersigwald
Copy link
Member

Are you worried about a mix of projects with ProduceReferenceAssembly=true/false irrespective of SDK/non-SDK?

Yes--but it gets worse if all SDK projects flip the switch.

What is it that might not be fixed?

Incremental build scenarios in VS like those fixed in the CPS project system by dotnet/project-system#2414 and its follow-ups.

@nguerrera
Copy link
Contributor

Incremental build scenarios in VS like those fixed in the CPS project system by dotnet/project-system#2414 and its follow-ups.

@panopticoncentral @Pilchie Are there such issues in the classic project system still unfixed? Are they tracked? Is it feasible to get them fixed in dev16?

@rainersigwald
Copy link
Member

Ok, I did a small proof-of-concept that things are still broken.

MixedSolutionWithRefAssemblies.zip

This solution has:

  • An SDK netstandard library that produces reference assemblies
  • A net472 .exe
  • A net472 unit test project

The unit test project tests library functionality by calling the app that calls the library.

  1. Open up the solution in VS and run the test through the UI.
========== Build: 3 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========
  1. It fails, because there's an error in the library. It subtracts instead of adds.
  2. Fix the error:
diff --git a/Lib/Class1.cs b/Lib/Class1.cs
index a33c4c0..76a1c90 100644
--- a/Lib/Class1.cs
+++ b/Lib/Class1.cs
@@ -6,7 +6,7 @@ namespace Lib
     {
         public static int Add(int a, int b)
         {
-            return a - b; // note deliberate error
+            return a + b;
         }
     }
 }
  1. Run the test again.
  2. The test still fails, because
========== Build: 1 succeeded, 0 failed, 2 up-to-date, 0 skipped ==========

That's because the fast-up-to-date check in csproj doesn't understand that it needs to tell MSBuild to build the app even though its direct input (the lib.dll reference assembly) hasn't changed. CoreCompile will be skipped, but the build will copy the implementation assembly along, so that the updated implementation is available at runtime.

@nguerrera
Copy link
Contributor

Thanks, @rainersigwald. I filed this as dotnet/project-system#4079, which we think from the discussion yesterd would be blocking this.

@Pilchie
Copy link
Member

Pilchie commented Oct 7, 2018

Ugh - I wasn't aware that there were still issues here :(

@nguerrera
Copy link
Contributor

dotnet/project-system#4079 is closed as won't fix. Does this mean we can never turn this on by default? :(

cc @tmeschter @jjmew @livarcocc

@tmeschter
Copy link
Contributor

Turning it on by default will only be a problem in solutions that mix SDK and non-SDK-style projects, and even then only while CSProj is the default project system for non-SDK-style projects.

We have to draw the line somewhere for new feature work in CSProj.

@panopticoncentral
Copy link

I don't remember all the details of ref assemblies off the top of my head, but would it be possible to have non-SDK projects not consume ref assemblies by default even if the project produces them? Then SDK projects could default to producing them and everything would work. It would also help in the situation where someone manually turns it on and breaks the up to date check for non-SDK projects.

@nguerrera nguerrera modified the milestones: 3.0.1xx, 5.0.1xx Sep 13, 2019
@nguerrera
Copy link
Contributor

@panopticoncentral That sounds like a good idea. It looks like the consuming side can set CompileUsingReferenceAssemblies=false if I read the code right just now. I'm not quite sure on the best way to make that default false for classic, true for sdk style. Thoughts, @rainersigwald? I'm slightly worried that this would be unexpected for someone that is already using ProduceReferenceAssembly with classic projects in the mix.

@rainersigwald
Copy link
Member

We could maybe set it to false in common.props for everyone, and rely on the SDK to set it to true? But that's a design change (it was intended as an opt-out, not opt-in, but we also didn't anticipate this problem, so . . .).

@dasMulli
Copy link
Contributor Author

So turning it to false would make build times slower for people already using it with the caveat that they never had working intellisense for their apps.
At least it shouldn't (🙏) break their builds if no further customization was performed.

@marcpopMSFT marcpopMSFT added the untriaged Request triage from a team member label Apr 16, 2020
@sfoslund sfoslund added needs team triage Requires a full team discussion and removed untriaged Request triage from a team member labels Apr 20, 2020
@sfoslund sfoslund removed their assignment Apr 20, 2020
@marcpopMSFT marcpopMSFT modified the milestones: 5.0.1xx, Backlog Apr 22, 2020
@marcpopMSFT marcpopMSFT removed the needs team triage Requires a full team discussion label Apr 29, 2020
@marcpopMSFT marcpopMSFT removed the Blocked Issue is blocked on an external dependency label Apr 29, 2020
@dasMulli
Copy link
Contributor Author

dasMulli commented May 5, 2020

Poking at this again - Could this be safely enabled for .NET 5 projects?

I guess people will be less likely to use .NET Standard projects so not doing anything for these would not run in to the classic project system errors since net5 projects cannot be referenced by .NET Framework.

@rainersigwald
Copy link
Member

@dasMulli your reasoning sounds correct to me, and I think that's a reasonable stake in the ground.

@dsplaisted dsplaisted modified the milestones: Backlog, 5.0.1xx May 11, 2020
@dsplaisted
Copy link
Member

In our sync-up, we decided to go ahead with enabling this by default for .NET 5.0 and higher.

@marcpopMSFT marcpopMSFT added the Partner request requests from partners label May 11, 2020
@nkolev92
Copy link
Contributor

Potentially related: NuGet/Home#4184

When NuGet adds support for packing ref assemblies into a package, we'd likely have to hinge off of ProduceReferenceAssembly=true in one way or another.

This change likely means NuGet would need another switch to pack ref assemblies.
It's a lesser concern, but just covering all the basis

@AraHaan
Copy link
Member

AraHaan commented May 15, 2020

Also related: I think .net core should move where it default places generated reference assemblies. When I tried to fix that issue on NuGet/Home, I got snagged where it would mistakenly replace the runtime version with the reference one as well which was no good at all just because the ref directory is under the one for the runtime version so moving them to $(OutDir)\ref\$(TargetFramework) seems to me to be a more sane approach to this for net5 or newer (or with them all).

NuGet/Home#4184 (comment)

@AraHaan
Copy link
Member

AraHaan commented May 15, 2020

Also another option is to force all old style csproj's to convert to sdk style to kill off the old style.

@pentp
Copy link

pentp commented Nov 11, 2020

Is this new default the root cause for all my projects getting these unwanted ref folders under $(OutDir)? Is this really the right place for these ref assemblies? Shouldn't these be under an entirely separate directory (next to obj and bin?) so that they don't get deployed together with actual binaries by default?

@dasMulli
Copy link
Contributor Author

These files shouldn't be part of the dotnet publish output so not end up in deployments. If so, I suggest opening new issues about that.

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

Successfully merging a pull request may close this issue.