Make Microsoft.NETCore.Platforms pack from CSProj #50468
Conversation
This ports the GenerateRuntimeGraph task from arcade as of https://github.com/dotnet/arcade/tree/d005a8ce6bb81129f9c3a4a907e1b7ccbf2504d5
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsBest reviewed commit-by-commit. First commit just makes the package build from CSProj without having the CSProj build anything. More tests will come in later PRs which make changes to the task. Related: #48507
|
|
|
||
| <ItemGroup> | ||
| <Content Include="runtime.json" PackagePath="/" /> | ||
| <Content Include="_._" PackagePath="lib/netstandard1.0" /> |
There was a problem hiding this comment.
curious on why we have this placeholder
There was a problem hiding this comment.
It ensures this package is installable in projects using packages.config.
| <TargetFrameworks>$(NetCoreAppToolCurrent);net472</TargetFrameworks> | ||
| <PackageId>$(MSBuildProjectName)</PackageId> | ||
| <AvoidRestoreCycleOnSelfReference>true</AvoidRestoreCycleOnSelfReference> | ||
| <AssemblyName>Microsoft.NETCore.Platforms.BuildTasks</AssemblyName> |
There was a problem hiding this comment.
whats the reason behind bringing this infra to runtime ?
There was a problem hiding this comment.
It’s only used by this single project in runtime. Splitting it into arcade is unnecessary and complicated changes. We should delete it from arcade after this. (Note that this helps with our pkgproj removal)
Can we instead put that task under the local repo tasks? https://github.com/dotnet/runtime/tree/main/src/tasks |
We could, but then we'd also need to come up with a way to test it. What do you imagine is the benefit of putting this in tasks? @ViktorHofer actually suggested otherwise in chat about this last week:
I'm not seeing a major issue with building this as part of libs. It's similar in a way to the source-generators will work. |
I was suggesting that cause that is where all the tasks we use as part of the build (that are not in arcade) live. So maybe we already have some code for the base task class, logger, etc that we could reuse. But you raise a good point, which is that we don't have tests for those tasks. |
|
In addition to tests I think the way the tasks build is somewhat undesirable. The build sequentially up front before anything else. The way this project is set up will make it build in parallel with all other libs. I'll see what I can do about sharing any common source by establishing a convention for it. |
|
Yeah makes sense. I think the purpose of this task with what you've explained makes more sense to put it under this package subtree. |
|
Looks like I didn't quite get the test condition right. I'll try another go at excluding browser. The CoreCLR failures are #50520. |
src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
Show resolved
Hide resolved
src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
Show resolved
Hide resolved
|
|
||
| <ItemGroup> | ||
| <Content Include="runtime.json" PackagePath="/" /> | ||
| <Content Include="_._" PackagePath="lib/netstandard1.0" /> |
There was a problem hiding this comment.
Why not move that "." file into a common location to be used by multiple packages?
There was a problem hiding this comment.
I considered that, but it is a zero byte file so net impact to the repo of sharing is zero (or might even be more depending on the length of the property name). If we find a lot of these floating around we could de-dup them to remove clutter, but I felt this was better.
| <ItemGroup> | ||
| <ProjectReference Include="..\src\Microsoft.NETCore.Platforms.csproj" /> | ||
| <!-- Workaround NuGet promoting this to a ProjectReference --> | ||
| <PackageReference Include="System.Memory" Condition="'$(TargetFramework)' == 'net472'" Version="$(SystemMemoryVersion)" /> |
There was a problem hiding this comment.
Can you please elaborate? Why does NuGet add a P2P for System.Memory?
There was a problem hiding this comment.
NuGet tried to restore System.Memory.csproj for net472 without this and restore failed. I believe it's because it's in the closure for the project added by the test targets. I think it's the same issue as NuGet/Home#9354.
| <TargetFrameworks>$(NetCoreAppToolCurrent);net472</TargetFrameworks> | ||
| <PackageId>$(MSBuildProjectName)</PackageId> | ||
| <AvoidRestoreCycleOnSelfReference>true</AvoidRestoreCycleOnSelfReference> | ||
| <AssemblyName>Microsoft.NETCore.Platforms.BuildTasks</AssemblyName> |
There was a problem hiding this comment.
Why do you need a different AssemblyName?
There was a problem hiding this comment.
I didn't need a different name, I wanted one. I didn't want an assembly floating around with the name Microsoft.NETCore.Platforms.
ghost
locked as resolved and limited conversation to collaborators
Best reviewed commit-by-commit.
First commit just makes the package build from CSProj without having the CSProj build anything.
Then I brought over the task from arcade and made the CSProj responsible for building that task (but not including it in the package). This is mostly just a direct port, fixing analyzer issues.
The remaining two commits are just some refactoring and a test.
More tests will come in later PRs which make changes to the task.
Related: #48507