[GitHub] [arrow] nealrichardson commented on a change in pull request #7183: ARROW-8586: [R] installation failure on CentOS 7

[GitBox <gi...@apache.org>]

nealrichardson commented on a change in pull request #7183:
URL: https://github.com/apache/arrow/pull/7183#discussion_r425440850



##########
File path: r/tools/linuxlibs.R
##########
@@ -80,45 +80,103 @@ identify_os <- function(os = Sys.getenv("LIBARROW_BINARY", Sys.getenv("LIBARROW_
     return(os)
   }
 
-  if (nzchar(Sys.which("lsb_release"))) {
-    distro <- tolower(system("lsb_release -is", intern = TRUE))
-    os_version <- system("lsb_release -rs", intern = TRUE)
-    # In the future, we may be able to do some mapping of distro-versions to
-    # versions we built for, since there's no way we'll build for everything.
-    os <- paste0(distro, "-", os_version)
-  } else if (file.exists("/etc/os-release")) {
-    os_release <- readLines("/etc/os-release")
-    vals <- sub("^.*=(.*)$", "\\1", os_release)
-    names(vals) <- sub("^(.*)=.*$", "\\1", os_release)
-    distro <- gsub('"', '', vals["ID"])
-    os_version <- "unknown" # default value
-    if ("VERSION_ID" %in% names(vals)) {
-      if (distro == "ubuntu") {
-        # Keep major.minor version
-        version_regex <- '^"?([0-9]+\\.[0-9]+).*"?.*$'
-      } else {
-        # Only major version number
-        version_regex <- '^"?([0-9]+).*"?.*$'
-      }
-      os_version <- sub(version_regex, "\\1", vals["VERSION_ID"])
-    } else if ("PRETTY_NAME" %in% names(vals) && grepl("bullseye", vals["PRETTY_NAME"])) {
-      # debian unstable doesn't include a number but we can map from pretty name
-      os_version <- "11"
+  linux <- distro()
+  if (is.null(linux)) {
+    cat("*** Unable to identify current OS/version\n")
+    return(NULL)
+  }
+  paste(linux$id, linux$short_version, sep = "-")
+}
+
+#### start distro ####

Review comment:
       In order to make the Linux distribution code more robust and extensible, I copied it out to a separate package and refactored so that I could add a test suite. See https://github.com/nealrichardson/distro.
   
   Currently, you can think of that repository as a mirror of the code in r/tools/linuxlibs.R, just put there so that I could test it. Ultimately I'd like to put that `distro` package on CRAN and have `arrow` depend on it (and delete these 90 lines of code here). It's not core functionality that belongs in `arrow`, it would be nice to update and maintain it separately, and it's probably of general interest. @wesm do you have an opinion about that?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org