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.
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
Adding OS and arch command line options #18889
Adding OS and arch command line options #18889
Changes from all commits
c4f2c66
7a2620d
9c43f94
3d453e2
5e88e2e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 assume we have some logic in the msbuild part of the SDK which already does this. Wouldn't it be cleaner to just set properties based on the command line options and let msbuild handle the actual RID selection then?
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.
Per the spec:
We could pass "private" properties to MSBuild if we had to, but if we can I think we should avoid it, as we'd have to prevent them from flowing across project references as global properties the same way we do with RuntimeIdentifier.
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.
Alternatively - if we have existing C# code which does this today, we should share that between MSBuild and CLI. If we don't, maybe we should consider doing so. Otherwise this will be a prime target for unintentional breaks and inconsistencies.
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.
For MSBuild, it's a property (
NETCoreSdkRuntimeIdentifier
) which we generate into the Microsoft.NETCoreSdk.BundledVersions.props file when we're building the SDK.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.
That sounds like something we could do for CLI as well - but it would require us to build the CLI as RID specific, which is not the case today.
Alternative idea - can we instead rely on the
dotnet.dll
location? It would be easier/safer than trying to compute the path. Options: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.
We already also use the
NETCoreSdkRuntimeIdentifierChain.txt
file for the workload MSBuild SDK resolver. So I think it's fine to rely on that file. I think looking next to dotnet.dll is a great idea to simplify this code, in what ways do you think it wouldn't be robust?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.
Does does the fact that these options have a forwarding function interfere here, since the runtime is passed to the RunCommand as a parameter instead of as additional MSBuild arguments like the other commands?
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.
We account for these options here: https://github.com/dotnet/sdk/pull/18889/files#diff-c59ee7ae48403cce44671ddc3549eb921e7c23bda26840c883b29d9f7e7942e5R38, the forwarding function shouldn't interfere.