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

Fix RHEL platform autodetection #383

Merged
merged 6 commits into from
Sep 11, 2024
Merged

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Sep 11, 2024

r-lib/pak#610

Can likely be done more elegantly but should go into the right direction and move things forward.

RHEL is reported as follows

pak::system_r_platform()
[1] "aarch64-unknown-linux-gnu-rhel-9.4"
>

pak::system_r_platform()
[1] "x86_64-pc-linux-gnu-rhel-9.4"
>

pak::system_r_platform()
[1] "aarch64-unknown-linux-gnu-rhel-8.10"
>

pak::system_r_platform()
[1] "x86_64-pc-linux-gnu-rhel-8.10"
>

Given that all RH-related sysdep mappings are referenced as "redhat", rewriting the respective internal mappings made most sense to me.

This change results in the following

sysreqs_install_plan("curl", config = list(sysreqs_platform = "x86_64-pc-linux-gnu-rhel-8.10"))

$os
[1] "linux"

$distribution
[1] "redhat"

$version
[1] "8"

$pre_install
character(0)

$install_scripts
character(0)

$post_install
character(0)

Given that "redhat-8" and "redhat-9" already work as desired when handed over manually, this change should align the platform parsing for the moment.
There might be other parts that need to be touched in addition, you might know best.

@gaborcsardi
Copy link
Member

Thanks! Have you thought about just adding rhel to the table in sysreqs2.R? I am a bit worried that we miss this conversion in the future. rhel is what the table should have used in the first place.

@pat-s
Copy link
Contributor Author

pat-s commented Sep 11, 2024

I thought about it but this would then be a breaking change as some users might have already hardcoded redhat or redhat-9 meanwhile to make the platform detection work.

Another option would be to have both for a while and then deprecate the old one. Your choice.

@gaborcsardi
Copy link
Member

Yeah, I meant keeping redhat and adding rhel.

@pat-s
Copy link
Contributor Author

pat-s commented Sep 11, 2024

Ah yeah, you said adding not replacing.

I'll change it then and add rhel. The version splitting might still be required?

@gaborcsardi
Copy link
Member

Thanks! I simplified it a bit and removed the extra logic.

@gaborcsardi gaborcsardi merged commit 13e8381 into r-lib:main Sep 11, 2024
10 checks passed
@pat-s
Copy link
Contributor Author

pat-s commented Sep 11, 2024

Ah, I was under the impression that the version needs to resolve to only a single digit for the major version but apparently some fuzzy matching is happening in the background 🙃

Thanks for cleaning up and merging so quickly!

@pat-s pat-s deleted the rhel-autodetection branch September 11, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants