Use ProjectReference to build against System.Private.CoreLib#38196
Use ProjectReference to build against System.Private.CoreLib#38196ViktorHofer merged 3 commits intodotnet:masterfrom
Conversation
|
Tagging subscribers to this area: @safern, @ViktorHofer |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| Exclude="$(RuntimePath)$(MSBuildProjectName).dll" /> | ||
| <ReferencePath Include="$(RuntimePath)System.*.dll;$(RuntimePath)Microsoft.Win32.*.dll;$(RuntimePath)netstandard.dll" | ||
| Exclude="$(RuntimePath)$(MSBuildProjectName).dll" /> | ||
| <ProjectReference Include="$(CoreLibProject)" /> |
|
Any impact to end-to-end library build perf with this change? |
|
This broke the steps to update reference sources: https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/updating-ref-source.md Building reference sources for System.Runtime produces an empty System.Runtime.cs. Rolling back this commit fixes it for me. |
|
I think we just need to update this condition to consider if the project has a P2P to System.Private.Corelib as well or if the project is a partial facade to pass down the runtime/src/libraries/Directory.Build.targets Line 168 in dd41a4a |
| <ItemGroup> | ||
| <ProjectReference> | ||
| <SkipGetTargetFrameworkProperties>true</SkipGetTargetFrameworkProperties> | ||
| <AdditionalProperties Condition="'%(Filename)' == 'System.Private.CoreLib' and '$(RuntimeFlavor)' == 'CoreCLR'">Configuration=$(CoreCLRConfiguration);TargetFramework=$(NetCoreAppCurrent)</AdditionalProperties> |
There was a problem hiding this comment.
When building the repository I believe this can cause double build of System.Private.CoreLib. Since it will have already been built without these global properties. It can also result in race conditions if CoreLib is being built in the same parallel project graph as traversal.
Do we need to specify TargetFramework? It looks like that's already set in S.P.C. Can't we just UndefineProperties TargetFramework instead so that it doesn't flow?
Can we avoid setting Configuration if it is already set?
Can we append to AdditionalProperties when setting?
There was a problem hiding this comment.
@ViktorHofer did you see this?
@ericstj yeah but forgot to respond, sorry.
Do we need to specify TargetFramework? It looks like that's already set in S.P.C. Can't we just UndefineProperties TargetFramework instead so that it doesn't flow?
Sure if that's possible. How would I prevent the property from flowing? Currently the TargetFramework Sdk controls that: https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets#L59. Do we need a hook in that sdk?
There was a problem hiding this comment.
Can we avoid setting Configuration if it is already set?
In which case would configuration already be set? We are not flowing that property.
There was a problem hiding this comment.
prevent the property from flowing
You can set the UndefineProperties metadata on the ProjectReference. If the config system doesn't honor it we might have to add a switch.
Global properties automatically flow. So Configuration will always be either 1) whatever the default is (eg: debug) or 2) specified as a global property by build.cmd|sh
In general we need to be extra careful about global property hygine.
|
@ViktorHofer @safern i can throw up a pr to fix up the generateReferenceSource, (if any of u is not actively working on one) |
|
Feel free, I'm not actively working on it. |
|
thanks Anirudh. |
Fixes #36464
Fixes #37963
Validated System.Private.CoreLib's ability to build incrementally (fast).