You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/04 17:36:03 UTC
[GitHub] [arrow] assignUser opened a new pull request, #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
assignUser opened a new pull request, #12790:
URL: https://github.com/apache/arrow/pull/12790
Building on @thisisnic 's work #12696
This requires less maintenance in the workflows but does add a `Suggests` on lintr
--
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
[GitHub] [arrow] assignUser commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1088897685
@thisisnic could you approve the checks please :)
--
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
[GitHub] [arrow] assignUser commented on a diff in pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
assignUser commented on code in PR #12790:
URL: https://github.com/apache/arrow/pull/12790#discussion_r842763947
##########
.github/workflows/r.yml:
##########
@@ -144,7 +144,7 @@ jobs:
# Don't set a TZ here to test that case. These builds will have the following warning in them:
# System has not been booted with systemd as init system (PID 1). Can't operate.
# Failed to connect to bus: Host is down
- archery docker run -e TZ="" r
+ archery docker run -e TZ="" -e GITHUB_ACTIONS="true" r
Review Comment:
Without this `lintr` does not detect being on GHA and no annotation will be generated.
--
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
[GitHub] [arrow] assignUser commented on a diff in pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
assignUser commented on code in PR #12790:
URL: https://github.com/apache/arrow/pull/12790#discussion_r842763075
##########
.github/workflows/r.yml:
##########
@@ -248,6 +248,7 @@ jobs:
env:
ARROW_R_CXXFLAGS: "-Werror"
_R_CHECK_TESTS_NLINES_: 0
+ GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN}}
Review Comment:
Yes, otherwise the windows installation from gh throws an error : https://github.com/assignUser/arrow/runs/5824319312?check_suite_focus=true#step:18:175
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1087830315
https://issues.apache.org/jira/browse/ARROW-16011
--
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
[GitHub] [arrow] assignUser commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1088756483
@thisisnic Without the correct `lintr` version the style test takes several minutes and produces a lot of false positives but even with the correct version it takes a minute or two. This is an issue I had not thought about and is imho not acceptable.
I thought using the test would make it easier but it actually needs several changes to the workflow and docker files to work properly. I will commit a new version soon.
--
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
[GitHub] [arrow] thisisnic commented on a diff in pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #12790:
URL: https://github.com/apache/arrow/pull/12790#discussion_r843702507
##########
r/R/array-data.R:
##########
@@ -44,7 +44,7 @@
ArrayData <- R6Class("ArrayData",
inherit = ArrowObject,
active = list(
- type = function() ArrayData__get_type(self),
+ type =function() ArrayData__get_type(self),
Review Comment:
```suggestion
type = function() ArrayData__get_type(self),
```
--
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
[GitHub] [arrow] thisisnic commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
thisisnic commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1090042370
Thanks for doing this @assignUser, this is really helpful! Looks good now, I'll remove the intentional CI failure and then merge once the CI passes.
--
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
[GitHub] [arrow] ursabot commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1090876529
Benchmark runs are scheduled for baseline = e047c9a6c9df565b86143036cc6bab26d3a59306 and contender = cf53e3cdcb4b2d68bc95f85b9ebcc0a212eb6ed1. cf53e3cdcb4b2d68bc95f85b9ebcc0a212eb6ed1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/34c166d12d964073ae1d0fff48b9058a...b0a274ea35724586acb6dc05d867377e/)
[Finished :arrow_down:0.13% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/b0a6426bb748495b8e2026660da092fe...e537e68a7c9841ec9805b5f749469b8f/)
[Failed :arrow_down:0.71% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/51da768e9bbd4b528127fa87588a1ce5...08a4472e3e0049a3adf1f0c76a51393d/)
[Finished :arrow_down:0.17% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ee3a40303662423ab0d608acdbc73885...b0586e421f99437fbce63acbef9de708/)
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/454| `cf53e3cd` ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/439| `cf53e3cd` test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/440| `cf53e3cd` ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/449| `cf53e3cd` ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/453| `e047c9a6` ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/438| `e047c9a6` test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/439| `e047c9a6` ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/448| `e047c9a6` ursa-thinkcentre-m75q>
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] assignUser commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1088636916
@thisisnic could you take a look at this?
--
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
[GitHub] [arrow] thisisnic commented on a diff in pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #12790:
URL: https://github.com/apache/arrow/pull/12790#discussion_r842736093
##########
r/tests/testthat/test-style.R:
##########
@@ -0,0 +1,22 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+test_that("Package is lint free", {
+ skip_on_cran()
+ skip_if_not_installed("lintr")
+ lintr::expect_lint_free()
Review Comment:
Nice, I didn't know this function existed before now!
##########
.github/workflows/r.yml:
##########
@@ -144,7 +144,7 @@ jobs:
# Don't set a TZ here to test that case. These builds will have the following warning in them:
# System has not been booted with systemd as init system (PID 1). Can't operate.
# Failed to connect to bus: Host is down
- archery docker run -e TZ="" r
+ archery docker run -e TZ="" -e GITHUB_ACTIONS="true" r
Review Comment:
Please can you just remind me what this does?
##########
.github/workflows/r.yml:
##########
@@ -248,6 +248,7 @@ jobs:
env:
ARROW_R_CXXFLAGS: "-Werror"
_R_CHECK_TESTS_NLINES_: 0
+ GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN}}
Review Comment:
Now you're using a different method to the one I started, is this line still needed?
##########
r/R/array-data.R:
##########
@@ -44,7 +44,7 @@
ArrayData <- R6Class("ArrayData",
inherit = ArrowObject,
active = list(
- type = function() ArrayData__get_type(self),
+ type =function() ArrayData__get_type(self),
Review Comment:
Just adding a comment here to remind us to remove this once the PR is ready to be approved
--
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
[GitHub] [arrow] assignUser commented on pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
assignUser commented on PR #12790:
URL: https://github.com/apache/arrow/pull/12790#issuecomment-1088877787
This should have failed but `lintr` could not find the package, need to set the working directory correctly.
--
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
[GitHub] [arrow] thisisnic closed pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
Posted by GitBox <gi...@apache.org>.
thisisnic closed pull request #12790: ARROW-16011: [R] CI jobs should fail if lintr picked up issues
URL: https://github.com/apache/arrow/pull/12790
--
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