This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Reclassify the test dependencies in corefx as toolset #34682
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericstj Can you comment on this? We can move this one back to product if need be.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'm move that back for now.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the values used in Version.Details.xml are kind of confusing, because
ProductDependencycould be a dependency needed in order to create your product.TestDependencymeans to me, something we use to test. Only core-setup is a test depedency in this case I believe.Coreclr would be more like
BuildDependencyand arcadeToolChainDependency.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's maybe even more like "MustShipThisOrElseDependency" and "EverythingElseDependency"
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.
Yeah those would also clarify really from the devs mind what that dependency is intended for.