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.

Sign up for GitHub

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

Jump to bottom

Improves check for Rust #182

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

Improves check for Rust #182

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4364bf8
improved rust check
alexhroom Sep 11, 2024
cb4a62f
Update src/library/tools/R/check.R
alexhroom Sep 12, 2024
17722c6
added checks for rustc and cargo in DESCRIPTION
alexhroom Sep 13, 2024
139b910
add fixed to grep
alexhroom Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/library/tools/R/check.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3925,15 +3925,16 @@ add_dummies <- function(dir, Log)

check_rust <- function()
{
## It is impossible to tell definiitively if a package
## compiles rust code. SystemRequirements in DESCRIPTION is
## fres-format, and only advisory. So we look at the
## installation log, which we found in check_src()
## A Cargo.toml file is used to identify a rust package,
## so check for Cargo.toml under src to see if rust code
## is compiled in the crate
if (is.na(InstLog)) return (NA)
srcd <- file.path(pkgdir, "src")
if (length(list.files(path = srcd, pattern = "Cargo.toml", recursive = TRUE)) == 0) return (NA)
##message("InstLog = ", InstLog)
lines <- readLines(InstLog, warn = FALSE)
l1 <- grep("(cargo build| Compiling )", lines)
if(!length(l1)) return(NA)
l1 <- grep(r"(cargo\N+build| Compiling )", lines)
Copy link
Member

Choose a reason for hiding this comment

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

Typo?

Suggested change
l1 <- grep(r"(cargo\N+build| Compiling )", lines)
l1 <- grep(r"(cargo\s+build| Compiling )", lines)

Copy link
Author

Choose a reason for hiding this comment

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

no: wanted to also capture configuration and toolchain use if CRAN lets that happen in future.

all the following are valid calls:
cargo build
cargo build
cargo +1.71 build (builds with specifically Rust version 1.71)
cargo +stable build (builds with current stable version)
cargo cargo --config profile.dev.package.image.opt-level=3 build (overrides a global config setting)

Copy link
Member

Choose a reason for hiding this comment

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

In the submission of the patch or in a comment I would mention this change allows to catch more cases of packages building rust. I am not sure R Core team or Brian Ripely is aware of this possibility.

But I was checking regex, and I only found that \N is:

The backreference ‘⁠\N⁠’, where ‘⁠N = 1 ... 9⁠’, matches the substring previously matched by the Nth parenthesized subexpression of the regular expression. (This is an extension for extended regular expressions: POSIX defines them only for basic ones.)

And as this backreference is inside the parenthesis I am not sure how it will work. That's why I proposed a change. When I want to match arbitrary characters between two known sets I usually use .+ because according to the documentation "The period ‘⁠.⁠’ matches any single character.".

Copy link

Choose a reason for hiding this comment

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

Looks like it's intended to mean the anything but a newline signifier in PCRE2. It will need perl = TRUE to work.

if(!length(l1)) return (NA)
l2 <- grep(" Compiling ", lines)
checkingLog(Log, "Rust compilation")
msg <- character(); OK <- TRUE
Expand All @@ -3949,6 +3950,22 @@ add_dummies <- function(dir, Log)
msg <- c(msg, "No rustc version reported prior to compilation")
## print(lines)
}
desc <- .read_description("DESCRIPTION")
sysreqs <- parse_description_field(desc, "SystemRequirements", default = FALSE)
if(!sysreqs) {
OK <- FALSE
msg <- c(msg, "Package compiles Rust but SystemRequirements not provided")
}
rustc_desc <- any(grep("rustc", sysreqs, ignore.case = TRUE))
cargo_desc <- any(grep("cargo", sysreqs, ignore.case = TRUE))
alexhroom marked this conversation as resolved.
Show resolved Hide resolved
if(!rustc_desc) {
OK <- FALSE
msg <- c(msg, "Package compiles Rust but rustc not in DESCRIPTION")
}
if(!cargo_desc) {
OK <- FALSE
msg <- c(msg, "Package compiles Rust but cargo not in DESCRIPTION")
}
if(OK)
resultLog(Log, "OK")
else {
Expand Down
Loading