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 / -targetRid as a build script argument #4413

Closed
tmds opened this issue May 21, 2024 · 10 comments · Fixed by dotnet/sdk#41540
Closed

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

tmds opened this issue May 21, 2024 · 10 comments · Fixed by dotnet/sdk#41540
Labels
area-product-experience Improvements in the end-user's product experience

Comments

@tmds
Copy link
Member

tmds commented May 21, 2024

dotnet/runtime#100580 removed the distro specific handling of rids in favor of distro packagers provide their rid when building .NET.

We should facilitate distro packagers in providing this value by making it a setting on the top-level build script.

cc @am11 @omajid @elinor-fung @ViktorHofer @mthalman

Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ViktorHofer
Copy link
Member

See dotnet/sdk#40843 (comment)

@ViktorHofer ViktorHofer changed the title add outputrid as a build script argument add --target-rid / -targetRid as a build script argument May 21, 2024
@ViktorHofer ViktorHofer changed the title add --target-rid / -targetRid as a build script argument Add --target-rid / -targetRid as a build script argument May 21, 2024
@tmds
Copy link
Member Author

tmds commented May 21, 2024

See dotnet/sdk#40843 (comment)

From that:

make --outputrid a required arg (maps to -p:TargetRID) to keep things really explicit

I don't think we should make it required as it forces anyone that calls build.sh to come up with a name.

outputrid CLI arg should probably just be called --target-rid / -targetRid.

nit: I prefer output-rid over target-rid because it names the output and it doesn't identify the target (except for the architecture).

Most package maintainers are expected to build for the build host architecture. It would be nice if they don't need to include the architecture in the argument value. Then the same package script works across architectures without the maintainer having to write some code to map the system architecture to the rid arch.

@am11
Copy link
Member

am11 commented May 21, 2024

nit: I prefer output-rid over target-rid because it names the output and it doesn't identify the target (except for the architecture).

Sounds good.

I don't think we should make it required as it forces anyone that calls build.sh to come up with a name.

It is not for everyone, only someone doing a (very targeted) non-portable build which is usually a distro maintainer running it via their CI/CD automation using a manifest file. ./build.sh --output-rid ubuntu.24-arm64. It is as explicit as it comes.

They can also use this helper if providing rid for non-portable build is a pain point:

#!/bin/sh

_distroId="$(grep -oP '^ID=\K.*' /etc/os-release | tr '[:upper:]' '[:lower:]')"
_version="$(grep -oP '^VERSION_ID="\K[0-9]+' /etc/os-release | tr -d '.')"
_arch="$(uname -m | { read -r arch; case $arch in x86_64) echo x64 ;; aarch64) echo arm64 ;; armv7*) echo arm ;; armv6*) echo arm ;; i*86) echo x86 ;; *) echo "$arch" ;; esac; })"

_outputRid="$_distroId.$_version${_version:+-}$_arch"

This will put the responsibility on a small subset of non-portable builders, yes, but it is trivial compared to dotnet infra maintaining the whole list of distros and their preferred style (no version, 1-part version, 2 or 3 parts). This will free them to change the format, and us free from maintenance.

@tmds
Copy link
Member Author

tmds commented May 21, 2024

It is not for everyone, only someone doing a (very targeted) non-portable build

I'm not sure what you mean exactly.

My preference is that a non-portable build uses a non-portable rid, and if a user wants he can control it using the argument. If the argument is not provided, the /etc/os-release based non-portable rid gets used.

@am11
Copy link
Member

am11 commented May 21, 2024

I'm not sure what you mean exactly.

You sounded like as if everyone will have to issue --output-rid argument, and I was clarifying it's not the case, it only applies to non-portable build.

If the argument is not provided, the /etc/os-release based non-portable rid gets used.

The problem with this is not all distros use the same scheme which I explained, so we will need to maintain the never-ending mappings. Is this little inconvenience really worth it? I don't think it is and we should just make --output-rid mandatory for non-portable builds. The error message can give a few examples to help user familiarize with various styles.

To me, the benefits of making the argument mandatory outweigh this convenience.

@tmds
Copy link
Member Author

tmds commented May 21, 2024

I don't think we should force everyone that does a non-portable build to have to understand and pick a rid when the build script can pick a very sensible value for it.

@am11
Copy link
Member

am11 commented May 21, 2024

OK. If your suggestion is to use $ID.$VERSION-$(uname -m), I agree. My rub is when we start to maintain this kind of mapping: distroA => majorVersion, distroB => majorAndMinorVersions, distroC => majorMinorPatchVersions, distroD => noVersion. That just opens a pandora box.

I think it would be reasonable to make lack of --output-rid defaults to just $ID.$VERSION-$(uname -m). WDYT?

@tmds
Copy link
Member Author

tmds commented May 21, 2024

Yes, what we have left in init-distro-rid.sh.

@tmds
Copy link
Member Author

tmds commented Jun 12, 2024

It would have been nicer if we had added the argument before removing the special distro handling from init-distro-rid.sh.

Now we require maintainers to figure out on their own how to get back to the rid they were building previously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-product-experience Improvements in the end-user's product experience
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants