Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address R CMD check _abort WARNING #419

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Address R CMD check _abort WARNING #419

wants to merge 12 commits into from

Conversation

JosiahParry
Copy link
Contributor

@JosiahParry JosiahParry commented Mar 25, 2025

R 4.5 a new check was introduces that identifies undesirable C function calls

https://github.com/r-devel/r-svn/blob/5695e158124051562158767089a55de80e7da452/src/library/tools/R/check.R#L3953-L3971

This has affected 24 packages on CRAN a good number of which use extendr.

Inspiration was taken from https://github.com/r-rust/hellorust/blob/9a3ab01fd7ce6b3e5ee24d5af5e8ca3b28abd71f/src/Makevars#L7

closes #418
sneaks in a resolution and closes #375

@JosiahParry JosiahParry marked this pull request as ready for review March 27, 2025 21:53
@JosiahParry JosiahParry enabled auto-merge (squash) March 28, 2025 17:58
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.16%. Comparing base (88ec630) to head (68ebbfc).

Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
R/use_extendr.R 99.08% <100.00%> (+0.13%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor Author

@JosiahParry JosiahParry left a comment

Choose a reason for hiding this comment

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

Commenting on major changes / additions for context

publish = FALSE,
version = "0.1.0",
edition = edition,
`rust-version` = "1.65"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This explicitly specifies the msrv for extendr to ensure that a version 4 cargo.lock file isn't generated.

Comment on lines +188 to +193
use_rextendr_template(
"config.R",
save_as = file.path("tools", "config.R"),
quiet = quiet,
overwrite = overwrite
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the config.R is used instead of a configure.win and configure file. Making it easier to maintain (plus its R which is just nicer to use imo).

@@ -198,6 +215,9 @@ use_extendr <- function(path = ".",
Sys.chmod("configure", "0755")
}

# Set the minimum version of R to 4.2 which is 64 bit
usethis::use_package("R", "Depends", "4.2")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set the minimum supported version of R in extendr explicitly.

Comment on lines +234 to +236
usethis::use_build_ignore(
file.path("src", "rust", "target")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensure no accidental incorporation of target files into a bundle. Has no effect other than preventing it from being included in the final tarball

@@ -1,9 +1,11 @@
TARGET_DIR = ./rust/target
LIBDIR = $(TARGET_DIR)/release
LIBDIR = $(TARGET_DIR)/@LIBDIR@
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is used to permit debug / release builds based on the DEBUG env var

STATLIB = $(LIBDIR)/lib{{{lib_name}}}.a
PKG_LIBS = -L$(LIBDIR) -l{{{lib_name}}}

all: C_clean
all: $(SHLIB) rust_clean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

used to clean up target and vendor directory which is the main resolution for the new WARNING

all: C_clean
all: $(SHLIB) rust_clean

.PHONY: $(STATLIB)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensures that the .so / .dll is always updated when being compiled e.g. register_extendr(compile = TRUE)

@@ -32,13 +34,13 @@ $(STATLIB):

export CARGO_HOME=$(CARGOTMP) && \
export PATH="$(PATH):$(HOME)/.cargo/bin" && \
RUSTFLAGS="$(RUSTFLAGS) --print=native-static-libs" cargo build @CRAN_FLAGS@ --lib --release --manifest-path=./rust/Cargo.toml --target-dir $(TARGET_DIR)
RUSTFLAGS="$(RUSTFLAGS) --print=native-static-libs" cargo build @CRAN_FLAGS@ --lib @PROFILE@ --manifest-path=./rust/Cargo.toml --target-dir $(TARGET_DIR)

# Always clean up CARGOTMP
rm -Rf $(CARGOTMP);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PROFILE@ is used to match the DEBUG env var

C_clean:
rm -Rf $(SHLIB) $(STATLIB) $(OBJECTS)
rust_clean:
rm -Rf $(CARGOTMP) $(VENDOR_DIR) @CLEAN_TARGET@
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CLEAN_TARGET@ is based on the DEBUG env var so that we do not clean the target directory in a debug build and permit incremental compilation.

Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

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

I trust in you and green checkmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants