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

Making the os_release function more strict #2657

Conversation

dmikushin
Copy link

Making the os_release CMake function more strict:

  1. Adding '=' to match pattern, because on Rocky Linux /etc/os-release contains not only ID="rocky", but also ID_LIKE="rhel centos fedora"
  2. Adding '| xargs', because on Rocky Linux ID="rocky" line contains extra quotes

1. Adding '=' to match pattern, because on Rocky Linux /etc/os-release contains not only ID="rocky", but also ID_LIKE="rhel centos fedora"
2. Adding '| xargs', because on Rocky Linux ID="rocky" contains extra quotes
@pmatilai pmatilai requested a review from dmnks September 13, 2023 07:17
@pmatilai
Copy link
Member

Hmm, wouldn't COMMAND sh -c ". /etc/os-release; echo ${var}" achieve the same thing, by letting the shell do the work instead?

@dmnks
Copy link
Contributor

dmnks commented Sep 13, 2023

Hmm, wouldn't COMMAND sh -c ". /etc/os-release; echo ${var}" achieve the same thing, by letting the shell do the work instead?

Yep, that's the "canonical" way to use /etc/os-release. For some reason, I didn't realize this when adding this cmake helper.

@dmikushin, feel free to update the PR to just source the file, otherwise I can do that later. Thanks!

@pmatilai
Copy link
Member

Heh, yeah sometimes the most obvious answer is just too obvious 😄

@dmnks
Copy link
Contributor

dmnks commented Sep 13, 2023

Indeed. It makes one wonder why the file is formatted in a shell-like fashion 😄 Also, I just noticed this is also what man os-release illustrates in the Example 3. Reading os-release in sh(1) section.

Copy link
Contributor

@dmnks dmnks left a comment

Choose a reason for hiding this comment

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

Mentioned above.

@pmatilai
Copy link
Member

pmatilai commented Oct 6, 2023

Merged via #2708 since there's been no activity on this PR. Thanks for the patch + report!

@pmatilai pmatilai closed this Oct 6, 2023
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.

None yet

3 participants