-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47704: [R] Update paths in nightly libarrow upload job #47727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
|
This unifies new line style (mixing CR LF and LF only -> LF only). Please use Can we test this without merge...? Do we need to merge to main to test this...? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess to test it, we could temporarily change it to run on PRs and see if it works here, and then change back before merging? I'm not opposed to merging and seeing if it works though, given the job isn't running anyway, so it won't make anything worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to merging and seeing if it works though, given the job isn't running anyway, so it won't make anything worse.
Same, I'm fine to merge this and confirm on main instead of (re)wiring to test from a branch.
| done | ||
| # New packages: repo/libarrow/${TARGET}-arrow-${VERSION}.zip | ||
| prune repo/libarrow/r-libarrow-darwin-arm64-openssl-1.1-* || : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL what || : does. I think I've used || true or the like for similar behavior elsewhere. Out of curiosity: is one better / more compatible / more idiomatic than the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tldr: no practical difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My cases:
- Use
truefor condition:- e.g.:
while true; do ...
- e.g.:
- Use
:for ignoring, doing nothing, ...:- e.g.:
... || :: Ignore error - e.g.:
: command line: Ignore this command line (same as# command linebut it works with multiline command line. I also useecho command linefor this case.) - e.g.:
if ...; do :; fi: Do nothing (likepassin Python) - e.g.:
: ${VARIABLE:=DEFAULT_VALUE}: Do nothing (setting default value by${...:=...}is the important part)
- e.g.:
|
After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit e5c8ccd. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change #45964 changed paths of pre-built Apache Arrow C++ binaries for R. But we forgot to update the nightly upload job. ### What changes are included in this PR? Update paths in the nightly upload job. ### Are these changes tested? No... ### Are there any user-facing changes? Yes. * GitHub Issue: #47704 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Nic Crane <[email protected]>
|
Failed...: |
|
Ah, the r-binary-packages job failed: |
…he#47727) ### Rationale for this change apache#45964 changed paths of pre-built Apache Arrow C++ binaries for R. But we forgot to update the nightly upload job. ### What changes are included in this PR? Update paths in the nightly upload job. ### Are these changes tested? No... ### Are there any user-facing changes? Yes. * GitHub Issue: apache#47704 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Nic Crane <[email protected]>
…47984) ### Rationale for this change PR #47727 refactored the R nightly upload workflow to handle r-pkg and r-lib files separately, but we needed to update the variable name used by `file.copy()`, causing the workflow to fail with "object 'current_path' not found" error. ### What changes are included in this PR? Add `current_path <- c(current_pkg_path, current_lib_path)` to combine the two path vectors before the `file.copy()` call on line 145. ### Are these changes tested? No ### Are there any user-facing changes? No * GitHub Issue: #47983 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>
Rationale for this change
#45964 changed paths of pre-built Apache Arrow C++ binaries for R. But we forgot to update the nightly upload job.
What changes are included in this PR?
Update paths in the nightly upload job.
Are these changes tested?
No...
Are there any user-facing changes?
Yes.