[GitHub] [arrow] nealrichardson commented on a diff in pull request #11666: ARROW-14638: [C++][R] Unknown C compiler / ccache on Arch Linux

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

nealrichardson commented on code in PR #11666:
URL: https://github.com/apache/arrow/pull/11666#discussion_r854608911


##########
.env:
##########
@@ -82,6 +82,8 @@ TZ=UTC
 
 # -1 does not attempt to install a devtoolset version, any positive integer will install devtoolset-n
 DEVTOOLSET_VERSION=-1
+# set to true to enable ccache build configuration
+R_BUILD_CCACHE=false

Review Comment:
   Remind me why we have to define this in .env and not just in the crossbow template?
   
   Unrelated but the block above L75-81 has gotten confused. The lines that should follow the comment are L77, L78, L80; the others should be moved away from the comment. 
   
   I also wonder if the env var should have a more clear name. Like, this is not just building with ccache, it's building it following the particular guidance of http://dirk.eddelbuettel.com/blog/2017/11/27/. As you point out in the other comment, we're still using ccache by default in the arrow build, regardless of the value here. 



##########
r/tools/nixlibs.R:
##########
@@ -301,8 +301,10 @@ build_libarrow <- function(src_dir, dst_dir) {
     # EXTRA_CMAKE_FLAGS will often be "", but it's convenient later to have it defined
     EXTRA_CMAKE_FLAGS = Sys.getenv("EXTRA_CMAKE_FLAGS"),
     # Make sure we build with the same compiler settings that R is using
-    CC = R_CMD_config("CC"),
-    CXX = paste(R_CMD_config("CXX11"), R_CMD_config("CXX11STD")),
+    # but we remove ccache, since `ARROW_USE_CCACHE` does that for our dependencies, and
+    # having ccache here as well leads to compile errors

Review Comment:
   A little more verbosity here
   
   ```suggestion
       # Exception: if you've added ccache to CC and CXX following
       # http://dirk.eddelbuettel.com/blog/2017/11/27/, some libarrow
       # third party dependencies will error on compilation. But don't
       # worry, `ARROW_USE_CCACHE=ON` by default, so if ccache
       # is found, it will be used by the libarrow build, and this does
       # not affect how R compiles the arrow bindings.
   ```



##########
r/tools/nixlibs.R:
##########
@@ -301,8 +301,10 @@ build_libarrow <- function(src_dir, dst_dir) {
     # EXTRA_CMAKE_FLAGS will often be "", but it's convenient later to have it defined
     EXTRA_CMAKE_FLAGS = Sys.getenv("EXTRA_CMAKE_FLAGS"),
     # Make sure we build with the same compiler settings that R is using
-    CC = R_CMD_config("CC"),
-    CXX = paste(R_CMD_config("CXX11"), R_CMD_config("CXX11STD")),
+    # but we remove ccache, since `ARROW_USE_CCACHE` does that for our dependencies, and
+    # having ccache here as well leads to compile errors
+    CC = gsub("^.*ccache", "", R_CMD_config("CC")),

Review Comment:
   You can just use `sub()`, we don't need to replace multiple matches



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