You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "assignUser (via GitHub)" <gi...@apache.org> on 2023/11/21 05:34:49 UTC
[PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
assignUser opened a new pull request, #38819:
URL: https://github.com/apache/arrow/pull/38819
### Rationale for this change
It hinders debug-ability for users if the failing log doesn't include all info by default.
### What changes are included in this PR?
Add debug output to test compile command in r/configure and always display output with regards to the binary download.
### Are these changes tested?
crossbow, locally
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #38819:
URL: https://github.com/apache/arrow/pull/38819#issuecomment-1820273543
@github-actions crossbow submit r-binary-packages
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38819:
URL: https://github.com/apache/arrow/pull/38819#issuecomment-1820275237
Revision: f6c06387ca4ec56ef1e2164335ac4a63998721fd
Submitted crossbow builds: [ursacomputing/crossbow @ actions-beee260f5b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-beee260f5b)
|Task|Status|
|----|------|
|r-binary-packages|[](https://github.com/ursacomputing/crossbow/actions/runs/6939755107/job/18877587036)|
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #38819:
URL: https://github.com/apache/arrow/pull/38819#issuecomment-1821170866
> My reading of nixlibs.R suggests that we will also get full failure logs if the libarrow source build fails...it might be worth double checking (or at least confirming that my reading matches yours 🙂 )
Yes, iirc we added that in a recent release. It will only show the cmake log when the compilation fails.
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38819:
URL: https://github.com/apache/arrow/pull/38819#issuecomment-1820271384
:warning: GitHub issue #38756 **has been automatically assigned in GitHub** to PR creator.
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #38819:
URL: https://github.com/apache/arrow/pull/38819#discussion_r1400733049
##########
r/tools/nixlibs.R:
##########
@@ -140,11 +136,11 @@ download_binary <- function(lib) {
checksum_ok <- system2(checksum_cmd, args = checksum_args)
if (checksum_ok != 0) {
- cat("*** Checksum validation failed for libarrow binary: ", libfile, "\n")
+ lg("Checksum validation failed for libarrow binary: %s/%s", lib, libfile)
Review Comment:
```suggestion
lg("Checksum validation failed for libarrow: %s/%s", lib, libfile)
```
##########
r/configure:
##########
@@ -413,7 +413,7 @@ CXX17FLAGS=`"${R_HOME}"/bin/R CMD config CXX17FLAGS`
CXX17STD=`"${R_HOME}"/bin/R CMD config CXX17STD`
CPPFLAGS=`"${R_HOME}"/bin/R CMD config CPPFLAGS`
TEST_CMD="${CXX17} ${CPPFLAGS} ${PKG_CFLAGS} ${CXX17FLAGS} ${CXX17STD} -xc++ -"
-echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} >/dev/null 2>&1
+TEST_ERROR=$(echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} -o /dev/null 2>&1)
Review Comment:
I think we might want something more like:
```
echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} -o /dev/null > test-compile.out 2>&1
```
...just because `$? -eq 0` is only related to the output of the previous command (right?). Even if it works with what you have here it might raise eyebrows later.
I also think you have to use backticks instead of `$()` because of bashisms.
(then below, ``TEST_ERROR=`cat test-compile.out` && rm test-compile.out`)
##########
r/tools/nixlibs.R:
##########
@@ -99,16 +99,12 @@ download_binary <- function(lib) {
libfile <- paste0("arrow-", VERSION, ".zip")
binary_url <- paste0(arrow_repo, "bin/", lib, "/arrow-", VERSION, ".zip")
if (try_download(binary_url, libfile)) {
- if (!quietly) {
lg("Successfully retrieved C++ binaries (%s)", lib)
Review Comment:
```suggestion
lg("Successfully retrieved libarrow (%s)", lib)
```
##########
r/tools/nixlibs.R:
##########
@@ -99,16 +99,12 @@ download_binary <- function(lib) {
libfile <- paste0("arrow-", VERSION, ".zip")
binary_url <- paste0(arrow_repo, "bin/", lib, "/arrow-", VERSION, ".zip")
if (try_download(binary_url, libfile)) {
- if (!quietly) {
lg("Successfully retrieved C++ binaries (%s)", lib)
- }
} else {
- if (!quietly) {
lg(
"Downloading libarrow binary failed for version %s (%s)\n at %s",
Review Comment:
```suggestion
"Downloading libarrow failed for version %s (%s)\n at %s",
```
##########
r/tools/nixlibs.R:
##########
@@ -140,11 +136,11 @@ download_binary <- function(lib) {
checksum_ok <- system2(checksum_cmd, args = checksum_args)
if (checksum_ok != 0) {
- cat("*** Checksum validation failed for libarrow binary: ", libfile, "\n")
+ lg("Checksum validation failed for libarrow binary: %s/%s", lib, libfile)
unlink(libfile)
libfile <- NULL
} else {
- cat("*** Checksum validated successfully for libarrow binary: ", libfile, "\n")
+ lg("Checksum validated successfully for libarrow binary: %s/%s", lib, libfile)
Review Comment:
```suggestion
lg("Checksum validated successfully for libarrow: %s/%s", lib, libfile)
```
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #38819:
URL: https://github.com/apache/arrow/pull/38819#issuecomment-1821974056
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 490cd83482594792de7e4d32ce83e8d4d56e1b96.
There were 2 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2023-11-21 21:23:26Z](https://conbench.ursa.dev/compare/runs/807b48a5bb79474faf72700ab37bd5e1...5cc8ab141ec444cfa451ab160379a33c/)
- [`file-write` (Python) with compression=lz4, dataset=fanniemae_2016Q4, file_type=feather, input_type=table](https://conbench.ursa.dev/compare/benchmarks/0655cc2ecb5773e38000c14dffde6091...0655d298a548794580009c88424835a8)
- [`dataset-serialize` (Python) with dataset=nyctaxi_multi_parquet_s3, format=csv, selectivity=1pc](https://conbench.ursa.dev/compare/benchmarks/0655cbb029b27eab8000778eb5d1f2e6...0655d219ceb7700d80006b3b46a2c2a7)
The [full Conbench report](https://github.com/apache/arrow/runs/18914707684) has more details.
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #38819:
URL: https://github.com/apache/arrow/pull/38819#discussion_r1400789017
##########
r/configure:
##########
@@ -413,7 +413,7 @@ CXX17FLAGS=`"${R_HOME}"/bin/R CMD config CXX17FLAGS`
CXX17STD=`"${R_HOME}"/bin/R CMD config CXX17STD`
CPPFLAGS=`"${R_HOME}"/bin/R CMD config CPPFLAGS`
TEST_CMD="${CXX17} ${CPPFLAGS} ${PKG_CFLAGS} ${CXX17FLAGS} ${CXX17STD} -xc++ -"
-echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} >/dev/null 2>&1
+TEST_ERROR=$(echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} -o /dev/null 2>&1)
Review Comment:
>I also think you have to use backticks instead of $() because of bashisms.
Nope I checked that with `checkbashism` the only thing flagged was the `echo "\n"` ^^
>the previous command
Yes and the previous command is the `$()` so unless we add something in between running the command and checking the exit code it will be fine (which is also the case for your solution).
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #38819:
URL: https://github.com/apache/arrow/pull/38819#discussion_r1400832242
##########
r/configure:
##########
@@ -413,7 +413,7 @@ CXX17FLAGS=`"${R_HOME}"/bin/R CMD config CXX17FLAGS`
CXX17STD=`"${R_HOME}"/bin/R CMD config CXX17STD`
CPPFLAGS=`"${R_HOME}"/bin/R CMD config CPPFLAGS`
TEST_CMD="${CXX17} ${CPPFLAGS} ${PKG_CFLAGS} ${CXX17FLAGS} ${CXX17STD} -xc++ -"
-echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} >/dev/null 2>&1
+TEST_ERROR=$(echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} -o /dev/null 2>&1)
Review Comment:
Nice, my knowledge here might be dated from a time that I had to make `configure` work with `sh` on Solaris 🤮
Maybe just add a comment that the `$()` doesn't affect the exit code checking on the next line? Mostly just because I'm not sure I'll remember that bash works that way the next time this fails 🙂
##########
r/configure:
##########
@@ -413,7 +413,7 @@ CXX17FLAGS=`"${R_HOME}"/bin/R CMD config CXX17FLAGS`
CXX17STD=`"${R_HOME}"/bin/R CMD config CXX17STD`
CPPFLAGS=`"${R_HOME}"/bin/R CMD config CPPFLAGS`
TEST_CMD="${CXX17} ${CPPFLAGS} ${PKG_CFLAGS} ${CXX17FLAGS} ${CXX17STD} -xc++ -"
-echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} >/dev/null 2>&1
+TEST_ERROR=$(echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} -o /dev/null 2>&1)
Review Comment:
Nice, my knowledge here might be dated from a time that I had to make `configure` work with `sh` on Solaris 🤮
Maybe just add a comment that the `$()` doesn't affect the exit code checking on the next line? Mostly just because I'm not sure I'll remember that bash works that way the next time this fails 🙂
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser merged PR #38819:
URL: https://github.com/apache/arrow/pull/38819
--
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
Re: [PR] GH-38756: [R] More debug output for r/configure and nixlibs.R [arrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #38819:
URL: https://github.com/apache/arrow/pull/38819#discussion_r1400791055
##########
r/tools/nixlibs.R:
##########
@@ -99,16 +99,12 @@ download_binary <- function(lib) {
libfile <- paste0("arrow-", VERSION, ".zip")
binary_url <- paste0(arrow_repo, "bin/", lib, "/arrow-", VERSION, ".zip")
if (try_download(binary_url, libfile)) {
- if (!quietly) {
lg("Successfully retrieved C++ binaries (%s)", lib)
Review Comment:
Makes sense to be consistent.
--
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