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.

Sign up for GitHub

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

Jump to bottom

Consider upstream-release to detect operating system #5414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Consider upstream-release to detect operating system #5414

wants to merge 1 commit into from

Conversation

scmanjarrez
Copy link

Fixes #3053

Changes proposed in this pull request:

  • Check /etc/upstream-release/lsb_release if present in system that are based on another "base" distributions like Mint

pkuczynski
pkuczynski requested changes Dec 23, 2023
View reviewed changes
Copy link
Member

@pkuczynski pkuczynski left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. Left some comments. Afterwards we can merge it I think.

scripts/functions/detect/system Outdated
@@ -21,7 +21,11 @@ __rvm_detect_system()

_system_type="Linux"

if [[ -f /etc/lsb-release ]] && __rvm_detect_system_from_lsb_release
if [[ -f /etc/upstream-release/lsb-release ]] && __rvm_detect_system_from_lsb_release upstream
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 we should implement it the other way around. First check the lsb_release and only when it returns 1 (fails), check the upstream-release.

Copy link
Author

Choose a reason for hiding this comment

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

The problem with that is lsb_release is also present in OS with upstream_release (i.e. mint), so it would enter that path returning incorrect value.

Copy link
Member

Choose a reason for hiding this comment

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

I see your point. If mint follows ubuntu, maybe we should just delete requirements/mint and remove it from lsb_release/__rvm_detect_system_from_lsb_release? This way __rvm_detect_system() would return 1 and the proposed check on upstream-release would work correctly.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, sorry for late response, I was out of home.
Mmmm, I don't know the implications of doing that, I don't know the code of rvm very well (I'm new to rvm code), if you think that's the way then I'll change the code as suggested

scripts/functions/detect/system_name/lsb_release Outdated
@@ -2,7 +2,11 @@

__rvm_detect_system_from_lsb_release()
{
local __system_name="$( awk -F'=' '$1=="DISTRIB_ID"{print $2}' /etc/lsb-release | head -n 1 | tr '[A-Z]' '[a-z]' | tr -d \" )"
local __lsb_release_file=""
if [ -n "$1" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

We should be more explicit here and compare against upstream.

CHANGELOG.md Outdated Show resolved Hide resolved
@pkuczynski pkuczynski changed the title Added condition to check upstream-release if present Consider upstream-release to detect operating system Dec 23, 2023
@pkuczynski pkuczynski added this to the rvm-1.29.13 milestone Dec 23, 2023
@scmanjarrez
Added condition to check upstream-release if present when detecting d…
130efc3 
…istribution

Update CHANGELOG.md

Co-authored-by: Piotr Kuczynski <piotr.kuczynski@gmail.com>

Applied changes as suggested
@scmanjarrez
Copy link
Author

scmanjarrez commented Feb 29, 2024 •
edited

I think I have updated the code as suggested. I have tested the execution using linux mint 21.2. I didn't remove the file requirements/mint because I'm not sure how it would affect the whole execution^1, feel free to remove if you think that's safe; I have removed https://github.com/rvm/rvm/pull/5414/files#diff-7fed6a7702987948fb50d49652528256f77236e30388aaaaaf53f17ecd908617L11 line, though.

  1. I couldn't follow the workflow until requirements/mint file.
est@test-virtual-machine:~/.rvm$ rvm install 3.2.2
Searching for binary rubies, this might take some time.
Found remote file https://rvm-io.global.ssl.fastly.net/binaries/ubuntu/22.04/x86_64/ruby-3.2.2.tar.bz2
Checking requirements for ubuntu.
Requirements installation successful.
ruby-3.2.2 - #configure
ruby-3.2.2 - #download
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32.2M  100 32.2M    0     0  28.6M      0  0:00:01  0:00:01 --:--:-- 28.6M
ruby-3.2.2 - #validate archive
ruby-3.2.2 - #extract
ruby-3.2.2 - #validate binary
ruby-3.2.2 - #setup
ruby-3.2.2 - #gemset created /home/test/.rvm/gems/ruby-3.2.2@global
ruby-3.2.2 - #importing gemset /home/test/.rvm/gemsets/global.gems..................................
ruby-3.2.2 - #generating global wrappers........
ruby-3.2.2 - #gemset created /home/test/.rvm/gems/ruby-3.2.2
ruby-3.2.2 - #importing gemsetfile /home/test/.rvm/gemsets/default.gems evaluated to empty gem list
ruby-3.2.2 - #generating default wrappers........
test@test-virtual-machine:~/.rvm$ cat /etc/lsb-release 
DISTRIB_ID=LinuxMint
DISTRIB_RELEASE=21.2
DISTRIB_CODENAME=victoria
DISTRIB_DESCRIPTION="Linux Mint 21.2 Victoria"
test@test-virtual-machine:~/.rvm$ cat /etc/upstream-release/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu Jammy Jellyfish"

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

@pkuczynski pkuczynski pkuczynski approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
rvm-1.29.13
Development

Successfully merging this pull request may close these issues.

Add binary rubies for Linux Mint
2 participants
@scmanjarrez @pkuczynski