GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr - #45524

Merged
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521
Feb 16, 2025
Merged

GH-45521: [CI][Dev][R] Install required cyclocomp package to be used with R lintr#45524
kou merged 7 commits into
apache:mainfrom
raulcd:GH-45521

Conversation

@raulcd

@raulcdraulcd commented Feb 13, 2025

Copy link
Copy Markdown
Member

Rationale for this change

The linting jobs are failing due to the new version of lintr not installing cyclocomp anymore.
We use cyclocomp but this is not part of the default linters of lintr anymore. We should install it individually.

What changes are included in this PR?

Install cyclocomp as part of setting up the linting environment for R on our linting job.
Pin old version of lintr for R Windows job as it otherwise fails with a lot of new linter issues.

Are these changes tested?

Yes via CI.

Are there any user-facing changes?

No

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #45521has been automatically assigned in GitHub to PR creator.

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Feb 13, 2025
@raulcd

Copy link
Copy Markdown
MemberAuthor

This fixes the lintr job on Dev linting but seems to fail on the linting for Windows R release:

 Error: Error: Not lint free
R/arrow-info.R:87:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:95:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:103:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:111:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:119:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:127:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/arrow-info.R:135:5: style: [return_linter] Use implicit return behavior; explicit return() is not needed.
return(FALSE)
^~~~~~
R/dplyr-arrange

This is due to a new default linter on the new lintr version:

  • New default linter return_linter()

I am unsure why this fails on this job and not the other one.

@raulcd

Copy link
Copy Markdown
MemberAuthor

return_linter seems to require many more changes as seen on other files failing. I am going to try and temporarily disable it.

@raulcd

Copy link
Copy Markdown
MemberAuthor

Once we disable return_linter there are other linters that fail:

 tests/testthat/test-dplyr-collapse.R:146:7: style: [commented_code_linter] Remove commented code.
# filter(dbl > 2) %>%
^~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:147:7: style: [commented_code_linter] Remove commented code.
# select(chr, int, lgl) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:148:7: style: [commented_code_linter] Remove commented code.
# mutate(twice = int * 2L) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:149:7: style: [commented_code_linter] Remove commented code.
# group_by(lgl) %>%
^~~~~~~~~~~~~~~~~
tests/testthat/test-dplyr-collapse.R:150:7: style: [commented_code_linter] Remove commented code.
# summarize(total = sum(int, na.rm = TRUE)) %>%
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In addition: Warning message:

I think the easier approach here is to temporarily pin lintr to the version we were using and open a new issue to unpin it and fix the new linters so we can fix CI which is broken at the moment.

@raulcd
raulcd marked this pull request as ready for review February 13, 2025 14:29
@raulcd

Copy link
Copy Markdown
MemberAuthor

@jonkeane it seems we run linting differently from the Windows R release job and the Dev linting job. On the Dev linting job installing cyclocomp is enough but for Windows we start getting a bunch of linting failures with the new version as seen on the comments above. I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this if we pin lintr to 3.1.2?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin here to 3.1.2, CRAN says is not available:

2025-02-13T13:40:04.1633160Z #15 0.292 > install.packages('lintr@3.1.2')
2025-02-13T13:40:04.3161054Z #15 0.294 Installing package into '/usr/local/lib/R/site-library'
2025-02-13T13:40:04.3161668Z #15 0.294 (as 'lib' is unspecified)
2025-02-13T13:40:11.3252442Z #15 7.454 Warning message:
2025-02-13T13:40:11.3253097Z #15 7.454 package 'lintr@3.1.2' is not available for this version of R
2025-02-13T13:40:11.3253653Z #15 7.454 2025-02-13T13:40:11.3254046Z #15 7.454 A version of this package for your version of R might be available elsewhere,
2025-02-13T13:40:11.3254443Z #15 7.454 see the ideas at
2025-02-13T13:40:11.3254875Z #15 7.454 https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages 2025-02-13T13:40:11.3255293Z #15 7.454 > 2025-02-13T13:40:11.3255459Z #15 7.454 > 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Feb 14, 2025
@jonkeane

Copy link
Copy Markdown
Member

I think we should do a follow up PR to fix those but we should probably merge this to fix CI as is currently broken for everyone. Let me know what do you think.

Strong agree that we should follow on later with fixes. Do we want a new issue or can we repurpose #45521 to be the follow on (and merge this PR as is)?

@raulcd

Copy link
Copy Markdown
MemberAuthor

kou
kou approved these changes Feb 14, 2025

@koukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

RUN echo "MAKEFLAGS=-j$(R -s -e 'cat(parallel::detectCores())')" >> $(R RHOME)/etc/Renviron.site
# We don't need arrow's dependencies, only lintr (and its dependencies)
RUN R -e "install.packages('lintr')"
RUN R -e "install.packages('cyclocomp')"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the .github/workflows/r.yml change isn't related to this change.

I thought that the pin in r.yml suppressed the lint error. But the lint error was caused by missing cyclocomp. (Found unused settings in config file (.lintr): unused_settings is just a warning. It's not an error.)

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Feb 14, 2025
@kou
kou merged commit 43d6f79 into apache:mainFeb 16, 2025
@koukou removed the awaiting merge Awaiting merge label Feb 16, 2025
@koukou mentioned this pull request Feb 16, 2025
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 43d6f79.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

amoeba pushed a commit that referenced this pull request Feb 21, 2025
…with R lintr (#45524)
### Rationale for this change
The linting jobs are failing due to the new version of `lintr` not installing `cyclocomp` anymore.
We use `cyclocomp` but this is not part of the default linters of `lintr` anymore. We should install it individually.
### What changes are included in this PR?
Install `cyclocomp` as part of setting up the linting environment for R on our linting job.
Pin old version of `lintr` for R Windows job as it otherwise fails with a lot of new linter issues.
### Are these changes tested?
Yes via CI.
### Are there any user-facing changes?
No
* GitHub Issue: #45521
Authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@raulcd@jonkeane@kou