-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-45605: [R][C++] Fix identifier ... preceded by whitespace warnings #45606
Conversation
@github-actions crossbow submit test-r-linux-as-cran |
Revision: c81bfb0 Submitted crossbow builds: ursacomputing/crossbow @ actions-4e0e69ed96
|
@github-actions crossbow submit test-r-linux-as-cran |
Revision: 27b318c Submitted crossbow builds: ursacomputing/crossbow @ actions-72b7eea56e
|
The clang20 job is failing due to not having latex (cf r-hub/containers#23) which we can ignore in this case, but won't be able to add it to the matrix if we need to ignore it — that's ok though, we just need to check this. It doesn't reproduce the failure we saw on CRAN but the clang20 image is out of date from r-hub. I pushed r-hub/containers#89 upstream to update the clang20 build, and then once that's failing can see if we get that same error. |
@github-actions crossbow submit test-r-linux-as-cran |
|
@github-actions crossbow submit test-r-linux-as-cran |
|
@github-actions crossbow submit test-r-linux-as-cran |
Revision: 1f5438d Submitted crossbow builds: ursacomputing/crossbow @ actions-c4c3a36e7c
|
@github-actions crossbow submit test-r-linux-as-cran clang20 has been updated, so this should run with a newer build (and hopefully also will fail) |
Revision: 56c2e9e Submitted crossbow builds: ursacomputing/crossbow @ actions-f272630e4b
|
@github-actions crossbow submit test-r-linux-as-cran And now with the fix? |
Revision: 03c12f1 Submitted crossbow builds: ursacomputing/crossbow @ actions-ba7fbc8d5c
|
^^^ Worked, the failures that persist are due to cpp11 which is resolved with r-lib/cpp11#448 |
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.
Thanks! I'll merge this in a sec here.
(It's totally ok that this is merged), but we might want to pull dev of cpp11 with r-lib/cpp11#448 when it merges in order for the new clang 20 job to pass cleanly |
👍. I filed #45616 for that. |
…#45606) ### Rationale for this change Fix warnings, pass on CRAN ### What changes are included in this PR? Removing some whitespace ### Are these changes tested? Yes ### Are there any user-facing changes? No, other than being releasable on CRAN * GitHub Issue: #45605 Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Bryce Mecum <[email protected]>
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit a01a812. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…rnings (apache#45606) ### Rationale for this change Fix warnings, pass on CRAN ### What changes are included in this PR? Removing some whitespace ### Are these changes tested? Yes ### Are there any user-facing changes? No, other than being releasable on CRAN * GitHub Issue: apache#45605 Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Bryce Mecum <[email protected]>
…rnings (apache#45606) ### Rationale for this change Fix warnings, pass on CRAN ### What changes are included in this PR? Removing some whitespace ### Are these changes tested? Yes ### Are there any user-facing changes? No, other than being releasable on CRAN * GitHub Issue: apache#45605 Authored-by: Jonathan Keane <[email protected]> Signed-off-by: Bryce Mecum <[email protected]>
Rationale for this change
Fix warnings, pass on CRAN
What changes are included in this PR?
Removing some whitespace
Are these changes tested?
Yes
Are there any user-facing changes?
No, other than being releasable on CRAN