Skip to content
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

Add --target-rid top level option #97

Closed
wants to merge 7 commits into from
Closed

Add --target-rid top level option #97

wants to merge 7 commits into from

Conversation

am11
Copy link
Member

@am11 am11 commented Jun 12, 2024

build.sh Outdated Show resolved Hide resolved
am11 and others added 2 commits June 12, 2024 09:24
Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
build.sh Outdated Show resolved Hide resolved
@@ -100,6 +102,10 @@ while [[ $# > 0 ]]; do
configuration=$2
shift
;;
-target-rid|-r)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-target-rid|-r)
-rid|-r)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought settled on --target-rid?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think target adds to the confusion of dotnet/source-build#4419.
For this top-level argument, imo we can just name it --rid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ViktorHofer thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer target-rid here with a good documentation explaining that this really just determines the name and nothing else. I feel that consistency is more important here.

Btw should we add the switch to the Windows script as well?

And last but not least this PR needs to target the sdk repo.

Copy link
Member

@tmds tmds Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, I know target is meant to be similar as how gcc has a build, host and target platform.
In contrast to that, a target rid does not describe a target platform. It is just a name.
I think that difference is what causes the confusion in dotnet/source-build#4419.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR needs to target the sdk repo.

@ViktorHofer how would that work? VMR build.sh is different than the one in SDK.

Copy link
Member

@ViktorHofer ViktorHofer Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dotnet/sdk/blob/main/src/SourceBuild/content/build.sh

We don't accept any PRs in this repository. This is just a mirror of code from various repos. Eventually the VMR will become its own source-of-truth but not until the beginning of .NET 10. See https://github.com/dotnet/dotnet?tab=readme-ov-file#contribution

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't knew it. Will close it.

@am11
Copy link
Member Author

am11 commented Jun 12, 2024

@tmds, I think we can get rid of calculation in this repo:

<PropertyGroup Label="CalculateRID">
It is only used for runtime where we have this calculation. Besides it's the old logic borrowed from runtime.

@tmds
Copy link
Member

tmds commented Jun 12, 2024

@tmds, I think we can get rid of calculation in this repo:

We need this in case the user doesn't specify a rid. This is responsible to make non-portable builds use the rid calculated by init-distro-rid.sh.

@am11
Copy link
Member Author

am11 commented Jun 12, 2024

Closing in favor of dotnet/sdk#41540.

@am11 am11 closed this Jun 12, 2024
@am11 am11 deleted the patch-3 branch June 12, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants