[GitHub] [arrow] jonkeane commented on a change in pull request #11000: ARROW-13757: [R] Fix download of C++ source for CRAN patch releases

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

jonkeane commented on a change in pull request #11000:
URL: https://github.com/apache/arrow/pull/11000#discussion_r695904613



##########
File path: r/tools/nixlibs.R
##########
@@ -215,10 +215,8 @@ download_source <- function() {
 
   # Given VERSION as x.y.z.p
   p <- package_version(VERSION)[1, 4]
-  if (is.na(p) || p < 1000) {
-    # This is either just x.y.z or it has a small (R-only) patch version
-    # Download from the official Apache release, dropping the p
-    VERSION <- as.character(package_version(VERSION)[1, -4])
+  if (is.na(p)) {

Review comment:
       Do we want to remove the p < 1000 checking here? I think we want to keep that (or maybe check < 9000 since we don't want dev versions to be attempted to be downloaded, right?

##########
File path: r/tools/nixlibs.R
##########
@@ -215,10 +215,8 @@ download_source <- function() {
 
   # Given VERSION as x.y.z.p
   p <- package_version(VERSION)[1, 4]
-  if (is.na(p) || p < 1000) {
-    # This is either just x.y.z or it has a small (R-only) patch version
-    # Download from the official Apache release, dropping the p
-    VERSION <- as.character(package_version(VERSION)[1, -4])
+  if (is.na(p)) {
+    # This is just x.y.z so download the official Apache release

Review comment:
       This is probably not the time to bikeshed, but I wonder if a try/catch (or "just" a modified if/else like below) would provide future proofing for the all-R no-cpp release that the removed comment hinted at.
   
   (I couldn't get GH's UI to let me edit all of these lines, but this would replace 219-224:
   
   ```
     # This is just x.y.z so download the official Apache release
     got_source <- apache_download(VERSION, tf1)
     # If this source isn't available, fall back to x.y.z (in case this is an R-only patch version that doesn't have a cpp patch
     if (!got_source) {
       VERSION <- as.character(package_version(VERSION)[1, -4])
       got_source <- apache_download(VERSION, tf1)    
     }
     
    if (got_source) {
       untar(tf1, exdir = src_dir)
       unlink(tf1)
       src_dir <- paste0(src_dir, "/apache-arrow-", VERSION, "/cpp")
    }
   ```




-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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