Skip to content

Also test on Linux ARM in R-CMD-check - #63

Merged
jwood000 merged 3 commits into
jwood000:mainfrom
jeroen:patch-1
Jun 6, 2026
Merged

Also test on Linux ARM in R-CMD-check#63
jwood000 merged 3 commits into
jwood000:mainfrom
jeroen:patch-1

Conversation

@jeroen

@jeroen jeroen commented May 30, 2026

Copy link
Copy Markdown
Contributor

Your package has some errors on Linux Arm that affects packages that depend on RcppAlgos. With this PR you should be able to reproduce the error on CI.

jeroen and others added 3 commits May 30, 2026 11:18
Rely on `parallel::detectCores()` for physical cores, falling back to
logical cores and then `1L` when detection fails. Normalize thread
limits to a positive integer with a `1L` fallback.
Reorder null/NA/invalid checks in `stdThreadMax()` and return an
explicit integer. Clarify `physicalCoreCount()` comments around fallback
behavior.

@jwood000 jwood000 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed my follow-up commit. The change is intentionally limited to removing the cross-OS shell fallback in startup-time core detection, which should address the linux-arm64 /bin/kstat warning while preserving the existing cache/thread heuristics. Pending CI.

@jwood000

jwood000 commented Jun 6, 2026

Copy link
Copy Markdown
Owner

@jeroen

Thanks for adding the linux-arm64 check. This immediately exposed a real package startup issue in .onLoad().

I traced the warning to physicalCoreCount() in R/zzz.R. The old fallback logic tried OS-specific shell commands for other platforms after the Linux-specific checks failed, which meant the Solaris /bin/kstat command could be attempted on linux-arm64. That explains the sh: 1: /bin/kstat: not found warning appearing during unrelated check stages.

I pushed a small follow-up commit to this PR branch so the new linux-arm64 check and the package-side fix can land together. Thanks again, this was a very helpful check to add.

@jwood000
jwood000 merged commit 28f883e into jwood000:main Jun 6, 2026
12 checks passed
@eddelbuettel

Copy link
Copy Markdown

@jwood000 Just FYI base R has this covered including on OSs where it is harder for us to get access:

> parallel::detectCores()
[1] 32
> 

@jwood000

jwood000 commented Jun 7, 2026

Copy link
Copy Markdown
Owner

@eddelbuettel

Thanks for the note. You’re absolutely right.

When I originally wrote this years ago, I was trying to be clever about estimating physical cores for an internal cache-size heuristic, but the result was more fragile than it needed to be. The linux-arm64 check exposed that pretty clearly.

The fix now removes the platform-specific shell-command fallback entirely and uses parallel::detectCores() with a simple fallback to 1L when needed. Much better aligned with the “don’t do harm” principle, which I picked up from your writings.

As always, I appreciate the feedback.

@eddelbuettel

Copy link
Copy Markdown

@jwood000 I am glad you are not taking it the wrong way but ... absolutely: been there, done that, and anything involving things "close to hardware" can be become a pain to maintainer especially under changing hardware environment (where we now have "three linux flavors": the eternal standard, but now arm64 and also musl/alpine with an inferior libc). Gets old quickly, and I have learned to appreciate the abstractions R gives me.

Sign up for free to 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