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 OS detection in a docker container #13172

Merged
merged 7 commits into from
Apr 22, 2020
Merged

Conversation

EchoPouet
Copy link
Contributor

@EchoPouet EchoPouet commented Jan 16, 2020

Hello,

I create this request to fix the OS detection in the docker containers. distros module use the command uname -a to get the OS name but in a docker container the value returned is not the container OS but the host OS.

So it's why I changed the code to use the command cat /etc/os-release | grep ^ID= to get the OS ID (see https://www.cyberciti.biz/faq/how-to-check-os-version-in-linux-command-line/). The OS ID is normally unique. An example with a CentOS container run in Ubuntu (the host):

root@bubuntu: docker run -t -i --rm centos bash
[root@95af48f1547c /]# uname -a
Linux 95af48f1547c 5.3.0-24-generic #26~18.04.2-Ubuntu SMP Tue Nov 26 12:34:22 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
[root@95af48f1547c /]# cat /etc/os-release | grep ^ID=
ID="centos"

I tested this fix in following OS:

  • Elementary,
  • Ubuntu,
  • Debian,
  • Fedora,
  • OpenMandriva,
  • CentOS
  • Alpine,
  • Mageia,
  • Zorin,
  • RedHat,
  • ArchLinux.

Also, I created a test to check if the OS detection is good. It's very simple but it exists.

Thanks

let dd = toLowerAscii($d)
result = dd in toLowerAscii(uname()) or dd in toLowerAscii(release()) or
("operating system: " & dd) in toLowerAscii(hostnamectl())
when defined(linux):
Copy link
Member

Choose a reason for hiding this comment

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

Huh? That cannot be correct for Haiku and the BSDs...

@Araq
Copy link
Member

Araq commented Jan 16, 2020

Really nice job, but please somehow ensure you only changed the logic for the OSes that you have tested it for.

@EchoPouet
Copy link
Contributor Author

Thank you but I don't know why I change the detection for Haiku and BSD. Maybe a merge error :)

@EchoPouet
Copy link
Contributor Author

Can I expected this fix merged in the next release ? Because I work every-time in docker (CI/CD use docker also) and I would like remove this fix in my code.

Thanks

@Araq
Copy link
Member

Araq commented Apr 20, 2020

I would love to merge it but please ensure the BSDs keep working.

@EchoPouet
Copy link
Contributor Author

Ho I missed your request, I'm going to test my change in BSD.

@Araq Araq merged commit dc40fb8 into nim-lang:devel Apr 22, 2020
@EchoPouet EchoPouet deleted the distros-docker branch June 14, 2020 09:27
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