-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Disable copying ProjectReferences in generator projects #74897
Disable copying ProjectReferences in generator projects #74897
Conversation
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue Detailsnull
|
Generator projects were getting all referenced projects as ReferenceCopyLocalPaths which causes the referenced library and all satellite assemblies to be copied to the output directory. We also default to including these into binplacing - the process that composes the shared framework layout and ref pack layouts. As a result we had multiple projects trying to copy Microsoft.Interop.SourceGeneration.* to the ref-pack. Two of those - JSImportGenerator.csproj and LibraryImportGenerator.csproj - could build in parallel - which causes a race condition binclash in the build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix. This shows the power and danger of binplacing into a common directory via a push instead of pull mechanism.
/backport to release/7.0-rc1 |
Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2971758851 |
Backporting as this just happened on a release/7.0 build: https://github.com/dotnet/runtime/pull/74920/checks?check_run_id=8135426753. |
@ericstj just double checking, doesn't this mean that the Line 35 in 6414a0b
Asking as runtime/src/libraries/NetCoreAppLibrary.props Line 186 in 6414a0b
EDIT: Also does this setting have any impact on source generators being packaged? |
We have the push tradeoff here because it enables a faster inner-loop for folks - less to rebuild in order to make changes to targeting-pack and test framework. It also enables distributed definition of composition. Both of these were major pieces of feedback we had for years in .NETFramework and DevDiv build - folks hated having to modify "authoring" to define the content of the product and complex set of steps (or full builds) to test. They wanted the definition of the product to be distributed and a local characteristic that they could control in their projects. Probably we can put more guards in place to catch binclashes wherever they might occur for whatever reason to protect us here. That would be a good change regardless since BinClashes can happen for any number of reasons.
Yep, that's a problem. We need to explicitly list every binary we want to ship. I'll make a PR that does that.
Both LibraryImportGenerator and JSImportGenerator set IsPackable=false. I tried setting IsPackable=true and these were not included in the package. Then I undid this change and tried again - tons of failures since the ProjectReference isn't preserving the resource subdirectory of sattelite dlls in the package.
Probably these should use the runtime/src/libraries/Directory.Build.targets Lines 230 to 232 in 3b706dd
Since they aren't currently packing, we can just remove this dead-code metadata on the project reference. I'll put that in the PR and then tag you guys to discuss. |
Any idea why the omission of that dependent assembly did not result in a build break. Don't we use the live built targeting pack's analyzers?
I was more interested in the analyzers that are already shipping inside a package ie the Json and the Extensions one. Are those still being included in their corrrsponding package? |
Yeah, those don't rely on project references inside the analyzer project to redistribute other projects. They use the AnalyzerReference mechanism from the src project which produces the package. |
No description provided.