Reclassify the test dependencies in corefx as toolset #34682
Conversation
These aren't actually repackaged/shipped, etc. so they can be made to be breaks in the graph. It might be good to introduce a new classification (test) but for now toolset is fine.
|
/cc @chcosta |
There was a problem hiding this comment.
/cc @danmosemsft @safern @ViktorHofer
Toolset/Product here are unfortunate names. Even test doesn't quite capture the distinction. To me the distinction is "reference + testing" vs "redistribution" but I still find that hard to reconcile with the actual usage and how folks think about it. Perhaps you should add a comment to describe the difference?
| @@ -25,8 +15,18 @@ | |||
| <Uri>https://github.com/dotnet/core-setup</Uri> | |||
| <Sha>464f69912c1e644e1b6e3aa0184f5c7c1a690cec</Sha> | |||
| </Dependency> | |||
| </ProductDependencies> | |||
| <ToolsetDependencies> | |||
| <Dependency Name="Microsoft.NETCore.Runtime.CoreCLR" Version="3.0.0-preview-27316-72"> | |||
There was a problem hiding this comment.
CoreCLR is indeed a product dependency right? We use it for our partial facades and as reference from some of our product assemblies. Unless the intent to use product dependency as “we ship these within our product”, is that the case?
There was a problem hiding this comment.
@ericstj Can you comment on this? We can move this one back to product if need be.
There was a problem hiding this comment.
@safern A better question here is this: If the exact version of coreclr mentioned here doesn't make it into the sdk/core-setup outputs, is there an issue?
There was a problem hiding this comment.
I think yes. Because we use CoreCLR to generate our facades and some projects use implementation details. So if we use a version that exposes and API and we use it in a project or generate a facade with that API, and then consume a previous CoreCLR version in core-setup that doesn’t contain that API I think it would be an issue, however I see that should be hard to happen.
There was a problem hiding this comment.
Okay I'm move that back for now.
There was a problem hiding this comment.
If the exact version of coreclr mentioned here doesn't make it into the sdk/core-setup outputs, is there an issue?
exact version no, we just need a version that has the same public types exposed from CoreLib.
We have no version dependency nor redistribution of coreclr. We only need one with the same public surface.
Historically we've allowed these repositories to build in parallel and I'd expect we'd want to do that moving forward.
There was a problem hiding this comment.
I think the values used in Version.Details.xml are kind of confusing, because ProductDependency could be a dependency needed in order to create your product.
TestDependency means to me, something we use to test. Only core-setup is a test depedency in this case I believe.
Coreclr would be more like BuildDependency and arcade ToolChainDependency.
There was a problem hiding this comment.
Or ReferenceDependency? I really feel like we need a document with some clear definitions of these terms and the behavior that falls out of using them.
There was a problem hiding this comment.
It's maybe even more like "MustShipThisOrElseDependency" and "EverythingElseDependency"
There was a problem hiding this comment.
Yeah those would also clarify really from the devs mind what that dependency is intended for.
|
In this case, it's not a huge issue to classify as product because coreclr is 'lower' in the stack. Really, product vs. toolset + whether the binary is shipping or non-shipping is being used to determine what we need actually publish in order to have a functional product |
This reverts commit 3f29522.
|
Back to all toolset then. I'll merge this in a second, |
Reclassify the test dependencies in corefx as toolset Commit migrated from dotnet/corefx@df75edc
These aren't actually repackaged/shipped, etc. so they can be made to be breaks in the graph.
It might be good to introduce a new classification (test) but for now toolset is fine.