os: add `kernel_version` #8381

SeekingMeaning · 2020-08-17T19:39:43Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?

This allows for e.g. `uname -r`.split(".").first in homebrew/core/gcc to be replaced with OS.kernel_version.major

Uses:

`uname -r`.split(".").first → OS.kernel_version.major
`uname -r`.to_i → OS.kernel_version.major
`uname -r`.chomp → OS.kernel_version
shell_output("uname -r").strip → OS.kernel_version
Utils.safe_popen_read("uname", "-r").chomp → OS.kernel_version

reitermarkus · 2020-08-17T20:02:35Z

Library/Homebrew/os.rb

@@ -13,6 +13,10 @@ def self.linux?
    RbConfig::CONFIG["host_os"].include? "linux"
  end

+  def self.uname_version


Maybe call this kernel_version?

MikeMcQuaid · 2020-08-17T20:34:40Z

This allows for e.g. `uname -r`.split(".").first in homebrew/core/gcc to be replaced with OS.uname_version.major

Any new utility methods that are used by formulae should be used by at least 3 (ideally a lot more than that) before we add them. I'm not sure I see the advantage here; it's not even actually shorter in this case?

SeekingMeaning · 2020-08-17T20:43:52Z

Any new utility methods that are used by formulae should be used by at least 3 (ideally a lot more than that) before we add them.

About 17 formulae use uname -r (including versioned gcc's)
See https://github.com/Homebrew/homebrew-core/search?q=%22uname+-r%22

it's not even actually shorter in this case?

Hehe, you might have a point 😆 It's longer but I think it's more clear

Bo98 · 2020-08-17T20:45:25Z

I'm not convinced the --build flag is needed in the case of GCC. That's usually indicative of a bad config.guess.

SeekingMeaning · 2020-08-17T21:41:31Z

Slightly off-topic, latest GNU config.guess supports arm64 Darwin now: https://git.savannah.gnu.org/gitweb/?p=config.git;a=commitdiff;h=2593751ef276497e312d7c4ce7fd049614c7bf80

MikeMcQuaid · 2020-08-18T07:57:18Z

Uses:

`uname -r`.split(".").first → OS.kernel_version.major

`uname -r`.to_i → OS.kernel_version.major

`uname -r`.chomp → OS.kernel_version

shell_output("uname -r").strip → OS.kernel_version

Utils.safe_popen_read("uname", "-r").chomp → OS.kernel_version

You've changed my mind: I like these.

MikeMcQuaid · 2020-08-18T07:58:22Z

Library/Homebrew/os.rb

@@ -13,6 +13,10 @@ def self.linux?
    RbConfig::CONFIG["host_os"].include? "linux"
  end

+  def self.kernel_version
+    @kernel_version ||= Version.new(`uname -r`.strip)


What if uname -r fails? (should probably raise)

Utils.popen_read might be a nicer fit here.

Also: could this get a unit test.

SeekingMeaning · 2020-08-18T18:21:20Z

What if uname -r fails? (should probably raise)

I'm not certain, but after checking in brew irb it seems that uname would print an error message (if it has a message to print) and then `uname -r` would return an empty string, so pretty much the same as Utils.popen_read but without silencing error messages

reitermarkus · 2020-08-18T20:15:58Z

Library/Homebrew/os.rb

+    @kernel_version ||= if output = Utils.popen_read("uname", "-r").chomp.presence
+      Version.new output
+    else
+      Version::NULL


Doesn't Version.create return Version::NULL?

That would be Version.parse but that might be excessive in this case since uname -r already returns a valid version string

reitermarkus · 2020-08-18T20:16:51Z

Library/Homebrew/os.rb

@@ -13,6 +13,14 @@ def self.linux?
    RbConfig::CONFIG["host_os"].include? "linux"
  end

+  def self.kernel_version
+    @kernel_version ||= if output = Utils.popen_read("uname", "-r").chomp.presence


Suggested change

@kernel_version ||= if output = Utils.popen_read("uname", "-r").chomp.presence

@kernel_version ||= if output = Utils.safe_popen_read("uname", "-r").chomp.presence

I don't think this should be able to fail, right?

Whoops, I forgot about that. I was just preserving the functionality of OS::Linux::Kernel.version since that's where this came from

We probably won't need the if since safe_popen_read raises an error anyways

SeekingMeaning · 2020-08-18T20:46:46Z

Library/Homebrew/os/linux/kernel.rb

-        return @version if @version
-
-        version = Utils.popen_read("uname", "-r").chomp
-        return Version::NULL unless version
-
-        @version = Version.new version
+        OS.kernel_version


I'm not sure if we want to preserve this or not

I think so unless you can verify it's unused in e.g. linuxbrew-core in which case: 🔥 it.

Looks like this is only used in brew in extend/os/linux/diagnostic (including below_minimum_version?):

% cd $(brew --repo) % grep -r 'Linux::Kernel' * Library/Homebrew/test/os/linux/diagnostic_spec.rb: allow(OS::Linux::Kernel).to receive(:below_minimum_version?).and_return(true) Library/Homebrew/extend/os/linux/diagnostic.rb: return unless OS::Linux::Kernel.below_minimum_version? Library/Homebrew/extend/os/linux/diagnostic.rb: Your Linux kernel #{OS::Linux::Kernel.version} is too old. Library/Homebrew/extend/os/linux/diagnostic.rb: We only support kernel #{OS::Linux::Kernel.minimum_version} or later.

% cd $(brew --repo homebrew/core) % git remote get-url origin https://github.com/Homebrew/linuxbrew-core % grep 'Linux::Kernel' Formula/* % grep 'Linux::' Formula/* Formula/clang-format.rb: if Formula["glibc"].any_version_installed? || OS::Linux::Glibc.system_version < Formula["glibc"].version Formula/glibc.rb: Glibc.version >= OS::Linux::Glibc.system_version Formula/glibc.rb: Your system's glibc version is #{OS::Linux::Glibc.system_version}, and Homebrew's glibc version is #{Glibc.version}. Formula/llvm@7.rb: if Formula["glibc"].any_version_installed? || OS::Linux::Glibc.system_version < Formula["glibc"].version Formula/llvm@8.rb: if Formula["glibc"].any_version_installed? || OS::Linux::Glibc.system_version < Formula["glibc"].version Formula/llvm@9.rb: if Formula["glibc"].any_version_installed? || OS::Linux::Glibc.system_version < Formula["glibc"].version Formula/llvm.rb: if Formula["glibc"].any_version_installed? || OS::Linux::Glibc.system_version < Formula["glibc"].version

Cool! I think you can 🔥 then.

So uh, what does 🔥 mean in this context? 😅

Sorry: burn it down (i.e. delete it)

MikeMcQuaid · 2020-08-19T10:27:50Z

Library/Homebrew/os.rb

@@ -13,6 +13,10 @@ def self.linux?
    RbConfig::CONFIG["host_os"].include? "linux"
  end

+  def self.kernel_version
+    @kernel_version ||= Version.new(Utils.safe_popen_read("uname", "-r").chomp)


reitermarkus reviewed Aug 17, 2020

View reviewed changes

SeekingMeaning force-pushed the os/uname_version branch from 527fd35 to 1c3c382 Compare August 17, 2020 20:54

SeekingMeaning changed the title ~~os: add uname_version~~ os: add kernel_version Aug 17, 2020

MikeMcQuaid reviewed Aug 18, 2020

View reviewed changes

reitermarkus reviewed Aug 18, 2020

View reviewed changes

SeekingMeaning commented Aug 18, 2020

View reviewed changes

MikeMcQuaid reviewed Aug 19, 2020

View reviewed changes

MikeMcQuaid approved these changes Aug 19, 2020

View reviewed changes

os: add kernel_version

3379e3b

SeekingMeaning force-pushed the os/uname_version branch from 460f1d9 to 3379e3b Compare August 19, 2020 18:42

SeekingMeaning merged commit f5b8cf4 into Homebrew:master Aug 19, 2020

SeekingMeaning deleted the os/uname_version branch August 19, 2020 19:40

SeekingMeaning mentioned this pull request Aug 19, 2020

Use OS.kernel_version Homebrew/homebrew-core#59902

Merged

5 tasks

BrewTestBot added the outdated PR was locked due to age label Dec 16, 2020

Homebrew locked as resolved and limited conversation to collaborators Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: add `kernel_version` #8381

os: add `kernel_version` #8381

SeekingMeaning commented Aug 17, 2020 •

edited

reitermarkus Aug 17, 2020

MikeMcQuaid commented Aug 17, 2020

SeekingMeaning commented Aug 17, 2020

Bo98 commented Aug 17, 2020

SeekingMeaning commented Aug 17, 2020

MikeMcQuaid commented Aug 18, 2020

MikeMcQuaid Aug 18, 2020

MikeMcQuaid Aug 18, 2020

SeekingMeaning commented Aug 18, 2020 •

edited

reitermarkus Aug 18, 2020

SeekingMeaning Aug 18, 2020

reitermarkus Aug 18, 2020

SeekingMeaning Aug 18, 2020

SeekingMeaning Aug 18, 2020

MikeMcQuaid Aug 19, 2020

SeekingMeaning Aug 19, 2020

MikeMcQuaid Aug 19, 2020

SeekingMeaning Aug 19, 2020

MikeMcQuaid Aug 19, 2020

MikeMcQuaid Aug 19, 2020

	@kernel_version \|\|= if output = Utils.popen_read("uname", "-r").chomp.presence
	@kernel_version \|\|= if output = Utils.safe_popen_read("uname", "-r").chomp.presence

os: add kernel_version #8381

os: add kernel_version #8381

Conversation

SeekingMeaning commented Aug 17, 2020 • edited

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 17, 2020

SeekingMeaning commented Aug 17, 2020

Bo98 commented Aug 17, 2020

SeekingMeaning commented Aug 17, 2020

MikeMcQuaid commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeekingMeaning commented Aug 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

os: add `kernel_version` #8381

os: add `kernel_version` #8381

SeekingMeaning commented Aug 17, 2020 •

edited

SeekingMeaning commented Aug 18, 2020 •

edited