r-lib / pkgdepends

R Package Dependency Resolution
https://r-lib.github.io/pkgdepends/
Other
102 stars 30 forks source link

Fix RHEL platform autodetection #383

Closed pat-s closed 2 months ago

pat-s commented 2 months ago

https://github.com/r-lib/pak/issues/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Yeah, I meant keeping redhat and adding rhel.

pat-s commented 2 months ago

Ah yeah, you said adding not replacing.

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

gaborcsardi commented 2 months ago

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

pat-s commented 2 months ago

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!