Skip to content

Conversation

am11
Copy link
Member

@am11 am11 commented Jun 12, 2024

@am11 am11 requested review from a team as code owners June 12, 2024 08:01
@ghost ghost added Area-Infrastructure untriaged Request triage from a team member labels Jun 12, 2024
@tmds
Copy link
Member

tmds commented Jun 12, 2024

Other than my comment about preferring --rid over --target-rid for the argument name, lgtm.

@am11
Copy link
Member Author

am11 commented Jun 12, 2024

It would be nice to reduce the disparity in terminologies because this situation is confusing and not very self-explanatory:

(source build)
--rid -> TargetRid
                  \
                --outputrid -> OutputRid -> CLI_CMAKE_PKG_RID
                  /
--outputrid -> __DistroRid
(regular build)

@am11
Copy link
Member Author

am11 commented Jun 12, 2024

All we really need is:

  • identifier to restore the packages during the build (host platform).
    • this is automatically deduced during the build and cannot be overridden.
  • identifier to build shipping packages for (target platform).
    • this is automatically deduced but it can be overridden from CLI.

@tmds
Copy link
Member

tmds commented Jun 12, 2024

It would be nice to reduce the disparity in terminologies because this situation is confusing and not very self-explanatory

  • TargetRid was decided on a name at some point, and I think OutputRid still has to be refactored to use that name.
  • __DistroRid is used to get a name from init-distro-rid.sh into the MSBuild side and determine a default TargetRid. I think we can keep the name as is, as it has a limited scope and the __ prefix suggest it is "private".

When we're adding top-level arguments to the vmr build script, I think we should name things so they are most clear to the package mantainers. They are supposed to figure out what to use without having to look at the implementation.

@am11 am11 changed the title Add --target-rid top level option Add --rid top level option Jun 12, 2024
@am11
Copy link
Member Author

am11 commented Jun 12, 2024

Changed to --rid. Another prospect is that we align it to dotnet build (which I personally don't find better than rid):

$ dotnet build --help | grep IDENT  
  -r, --runtime <RUNTIME_IDENTIFIER>   The target runtime to build for.

@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 12, 2024

I'm fine with calling it --rid but please also add the option to specify --target-rid and document it because that's the name of the underlying property in msbuild. We shouldn't allow the -r short form because that's already reserved for restore. The VMR script doesn't yet have that option but we will eventually add it.

@ViktorHofer ViktorHofer enabled auto-merge (squash) June 12, 2024 17:52
@am11
Copy link
Member Author

am11 commented Jun 12, 2024

Windows aspnetcore failure is unrelated (failing on other PRs too).

@am11 am11 closed this Jun 13, 2024
auto-merge was automatically disabled June 13, 2024 05:29

Pull request was closed

@am11 am11 reopened this Jun 13, 2024
@ViktorHofer ViktorHofer merged commit 3ed3bf6 into dotnet:main Jun 13, 2024
@am11 am11 deleted the patch-2 branch June 13, 2024 07:29
@tmds
Copy link
Member

tmds commented Jun 13, 2024

@am11, thank you for picking this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Infrastructure untriaged Request triage from a team member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add --target-rid / -targetRid as a build script argument

3 participants