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 2021/07/26 20:16:25 UTC
[GitHub] [arrow] jonkeane opened a new pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
jonkeane opened a new pull request #10805:
URL: https://github.com/apache/arrow/pull/10805
This does three things:
* Adds a linting step to the dev workflow that uses {lintr} to check that the R package lints correctly
* Adds an autotune step that uses {styler} to autostyle R code such that it should then lint fine
* Makes the slew of changes needed for the package to lint cleanly — there's a huge delta here, but it is almost all whitespace changes
Given the wide-reaching nature of these changes, it does already conflict with changes we've merged in. I am happy to go through, rebase, fix all that needs to be fixed and then merge quickly so we don't continue conflicting once we decide this is a good way to go.
--
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] jonkeane edited a comment on pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-887000651
Here is what an autotune looks like:
https://github.com/jonkeane/arrow/runs/3165418070?check_suite_focus=true#step:11:232
And a lint errors do not fail the build, but use GitHub annotations that will display both in the actions UI *and* as annotations in the PR files view. There are duplicate annotations since we run the dev workflow both on PR and on push, we might be able to filter one of those away or disable it if we wanted
https://github.com/jonkeane/arrow/pull/8/files#diff-79100695986bbd6a63704fe9f238ce3ae9a39ddd093b7f6b213d4a722309d20aR848
--
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] jonkeane commented on pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-891252858
Ok, I've added a section to dev docs + listed all the PRs/issues on the lintr front
--
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] nealrichardson commented on a change in pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#discussion_r681069081
##########
File path: ci/docker/linux-apt-lint.dockerfile
##########
@@ -35,9 +35,40 @@ RUN apt-get update && \
python3-dev \
python3-pip \
ruby \
+ apt-transport-https \
+ software-properties-common \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*
+ARG r=4.1
+RUN apt-key adv \
+ --keyserver keyserver.ubuntu.com \
+ --recv-keys E298A3A825C0D65DFD57CBB651716619E084DAB9 && \
+ # NOTE: R 3.5 and 3.6 are available in the repos with -cran35 suffix
+ # for trusty, xenial, bionic, and eoan (as of May 2020)
+ # -cran40 has 4.0 versions for bionic and focal
+ # R 3.2, 3.3, 3.4 are available without the suffix but only for trusty and xenial
+ # TODO: make sure OS version and R version are valid together and conditionally set repo suffix
+ # This is a hack to turn 3.6 into 35, and 4.0/4.1 into 40:
+ add-apt-repository 'deb https://cloud.r-project.org/bin/linux/ubuntu '$(lsb_release -cs)'-cran'$(echo "${r}" | tr -d . | tr 6 5 | tr 1 0)'/' && \
+ apt-get install -y \
+ r-base=${r}* \
+ r-recommended=${r}* \
+ libxml2-dev
+
+# Ensure parallel R package installation, set CRAN repo mirror,
+# and use pre-built binaries where possible
+COPY ci/etc/rprofile /arrow/ci/etc/
+RUN cat /arrow/ci/etc/rprofile >> $(R RHOME)/etc/Rprofile.site
+# Also ensure parallel compilation of C/C++ code
+RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Makeconf
+
+
+COPY ci/scripts/r_deps.sh /arrow/ci/scripts/
+COPY r/DESCRIPTION /arrow/r/
+RUN /arrow/ci/scripts/r_deps.sh /arrow
Review comment:
Does the arrow package need to be installable in order to lint it? (I would hope not.)
--
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] jonkeane closed pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #10805:
URL: https://github.com/apache/arrow/pull/10805
--
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] jonkeane commented on a change in pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#discussion_r681172357
##########
File path: ci/docker/linux-apt-lint.dockerfile
##########
@@ -35,9 +35,40 @@ RUN apt-get update && \
python3-dev \
python3-pip \
ruby \
+ apt-transport-https \
+ software-properties-common \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*
+ARG r=4.1
+RUN apt-key adv \
+ --keyserver keyserver.ubuntu.com \
+ --recv-keys E298A3A825C0D65DFD57CBB651716619E084DAB9 && \
+ # NOTE: R 3.5 and 3.6 are available in the repos with -cran35 suffix
+ # for trusty, xenial, bionic, and eoan (as of May 2020)
+ # -cran40 has 4.0 versions for bionic and focal
+ # R 3.2, 3.3, 3.4 are available without the suffix but only for trusty and xenial
+ # TODO: make sure OS version and R version are valid together and conditionally set repo suffix
+ # This is a hack to turn 3.6 into 35, and 4.0/4.1 into 40:
+ add-apt-repository 'deb https://cloud.r-project.org/bin/linux/ubuntu '$(lsb_release -cs)'-cran'$(echo "${r}" | tr -d . | tr 6 5 | tr 1 0)'/' && \
+ apt-get install -y \
+ r-base=${r}* \
+ r-recommended=${r}* \
+ libxml2-dev
+
+# Ensure parallel R package installation, set CRAN repo mirror,
+# and use pre-built binaries where possible
+COPY ci/etc/rprofile /arrow/ci/etc/
+RUN cat /arrow/ci/etc/rprofile >> $(R RHOME)/etc/Rprofile.site
+# Also ensure parallel compilation of C/C++ code
+RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Makeconf
+
+
+COPY ci/scripts/r_deps.sh /arrow/ci/scripts/
+COPY r/DESCRIPTION /arrow/r/
+RUN /arrow/ci/scripts/r_deps.sh /arrow
Review comment:
I'll add a comment here that we might be able to remove it if lintr changes (there are a few different PRs + issues I've sent on this, and it would take all of them accepted to not need to do this anymore)
And I'll add a comment to where I install from my fork listing the PRs/issues that need resolved + merged before we can use pure 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] nealrichardson commented on a change in pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#discussion_r678522515
##########
File path: r/data-raw/codegen.R
##########
@@ -82,7 +84,7 @@ wrap_call <- function(name, return_type, args) {
feature_available <- function(feat) {
glue::glue(
-'extern "C" SEXP _{feat}_available() {{
+ 'extern "C" SEXP _{feat}_available() {{
Review comment:
```suggestion
'extern "C" SEXP _{feat}_available() {{
```
--
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] nealrichardson commented on a change in pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#discussion_r681169743
##########
File path: ci/docker/linux-apt-lint.dockerfile
##########
@@ -35,9 +35,40 @@ RUN apt-get update && \
python3-dev \
python3-pip \
ruby \
+ apt-transport-https \
+ software-properties-common \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*
+ARG r=4.1
+RUN apt-key adv \
+ --keyserver keyserver.ubuntu.com \
+ --recv-keys E298A3A825C0D65DFD57CBB651716619E084DAB9 && \
+ # NOTE: R 3.5 and 3.6 are available in the repos with -cran35 suffix
+ # for trusty, xenial, bionic, and eoan (as of May 2020)
+ # -cran40 has 4.0 versions for bionic and focal
+ # R 3.2, 3.3, 3.4 are available without the suffix but only for trusty and xenial
+ # TODO: make sure OS version and R version are valid together and conditionally set repo suffix
+ # This is a hack to turn 3.6 into 35, and 4.0/4.1 into 40:
+ add-apt-repository 'deb https://cloud.r-project.org/bin/linux/ubuntu '$(lsb_release -cs)'-cran'$(echo "${r}" | tr -d . | tr 6 5 | tr 1 0)'/' && \
+ apt-get install -y \
+ r-base=${r}* \
+ r-recommended=${r}* \
+ libxml2-dev
+
+# Ensure parallel R package installation, set CRAN repo mirror,
+# and use pre-built binaries where possible
+COPY ci/etc/rprofile /arrow/ci/etc/
+RUN cat /arrow/ci/etc/rprofile >> $(R RHOME)/etc/Rprofile.site
+# Also ensure parallel compilation of C/C++ code
+RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Makeconf
+
+
+COPY ci/scripts/r_deps.sh /arrow/ci/scripts/
+COPY r/DESCRIPTION /arrow/r/
+RUN /arrow/ci/scripts/r_deps.sh /arrow
Review comment:
Could you add a comment linking to the PR that would let us remove it?
--
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 #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-886997254
https://issues.apache.org/jira/browse/ARROW-13326
--
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] jonkeane edited a comment on pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane edited a comment on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-887000651
Here is what an autotune looks like:
https://github.com/jonkeane/arrow/runs/3165418070?check_suite_focus=true#step:11:232
And a lint errors do not fail the build, but use GitHub annotations that will display both in the actions UI *and* as annotations in the PR files view. [1]
https://github.com/jonkeane/arrow/pull/8/files#diff-79100695986bbd6a63704fe9f238ce3ae9a39ddd093b7f6b213d4a722309d20aR848
[1] – There are duplicate annotations because that PR is from a branch on my fork back to my fork which triggers both push and PR workflows. With PRs to apache/arrow only the PR workflow will be triggered so there won't be duplicates then.
--
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] jonkeane commented on a change in pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#discussion_r681118564
##########
File path: ci/docker/linux-apt-lint.dockerfile
##########
@@ -35,9 +35,40 @@ RUN apt-get update && \
python3-dev \
python3-pip \
ruby \
+ apt-transport-https \
+ software-properties-common \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*
+ARG r=4.1
+RUN apt-key adv \
+ --keyserver keyserver.ubuntu.com \
+ --recv-keys E298A3A825C0D65DFD57CBB651716619E084DAB9 && \
+ # NOTE: R 3.5 and 3.6 are available in the repos with -cran35 suffix
+ # for trusty, xenial, bionic, and eoan (as of May 2020)
+ # -cran40 has 4.0 versions for bionic and focal
+ # R 3.2, 3.3, 3.4 are available without the suffix but only for trusty and xenial
+ # TODO: make sure OS version and R version are valid together and conditionally set repo suffix
+ # This is a hack to turn 3.6 into 35, and 4.0/4.1 into 40:
+ add-apt-repository 'deb https://cloud.r-project.org/bin/linux/ubuntu '$(lsb_release -cs)'-cran'$(echo "${r}" | tr -d . | tr 6 5 | tr 1 0)'/' && \
+ apt-get install -y \
+ r-base=${r}* \
+ r-recommended=${r}* \
+ libxml2-dev
+
+# Ensure parallel R package installation, set CRAN repo mirror,
+# and use pre-built binaries where possible
+COPY ci/etc/rprofile /arrow/ci/etc/
+RUN cat /arrow/ci/etc/rprofile >> $(R RHOME)/etc/Rprofile.site
+# Also ensure parallel compilation of C/C++ code
+RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Makeconf
+
+
+COPY ci/scripts/r_deps.sh /arrow/ci/scripts/
+COPY r/DESCRIPTION /arrow/r/
+RUN /arrow/ci/scripts/r_deps.sh /arrow
Review comment:
The arrow package does not, but the dependencies do for resolving their namespaces for the object name lintr. I've got a few issues / PRs to lintr that I've submitted / will submit that might make that not necessary, but at least with how it works now, the deps need to be installed in order to access their namespaces.
--
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] jonkeane commented on pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-886998223
I staged the commits such that[ the first commit is all of the changes to make the package lint cleanly](https://github.com/apache/arrow/pull/10805/commits/7eeef187d2e9da2b96ac736fca0a1e8bf56f38f1) [the second commit are the changes that {styler} introduced](https://github.com/apache/arrow/pull/10805/commits/253d1b79ae88ff8f2489d10765d775869c2249fa) and the third commit is making the autotune work 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] nealrichardson commented on pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-891226869
I'm good with proceeding with this. If it becomes burdensome or creates too many failed builds, we can revisit, but I am generally in favor of automating away things like styling.
Could you add a note to the developer documentation explaining the styling and give instruction on how to style correctly? (R command, instructions for IDEs to add styling on save, precommit hook, run autotune, etc.)
--
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] jonkeane commented on pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
jonkeane commented on pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#issuecomment-887000651
Here is what an autotune looks like:
https://github.com/jonkeane/arrow/runs/3165418070?check_suite_focus=true#step:11:232
And a lint error:
https://github.com/jonkeane/arrow/runs/3165130725?check_suite_focus=true#step:7:5122
--
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] nealrichardson commented on a change in pull request #10805: ARROW-13326: [R] [Archery] Add linting to dev CI
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10805:
URL: https://github.com/apache/arrow/pull/10805#discussion_r678522718
##########
File path: r/data-raw/codegen.R
##########
@@ -58,7 +58,9 @@ get_exported_functions <- function(decorations, export_tag) {
out <- decorations %>%
filter(decoration %in% paste0(export_tag, "::export")) %>%
mutate(functions = map(context, decor:::parse_cpp_function)) %>%
- { vec_cbind(., vec_rbind(!!!pull(., functions))) } %>%
+ {
+ vec_cbind(., vec_rbind(!!!pull(., functions)))
+ } %>%
Review comment:
```suggestion
vec_cbind(., vec_rbind(!!!pull(., functions))) %>%
```
--
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