Improves check for Rust

[alexhroom]

yesterday a check for packages which compile Rust was added: 6114d41

This check claims that "It is impossible to tell definiitively if a package compiles rust code". This is not true. Rust code can be compiled if and only if there is a Cargo.toml metadata file. This PR changes the check_rust check to quit if the package does not contain a Cargo.toml anywhere under the src directory.

[yutannihilation]

Rust code can be compiled if and only if there is a Cargo.toml metadata file.

I agree this is true in most of the cases, but maybe there's a little chance that Cargo.toml is dynamically generated?

[alexhroom]

@yutannihilation I suppose - I've also had it pointed out to me that if somebody really wanted to then they could write Rust code directly inside the Makevars file and build it in there. currently thinking if there's anything more reliable?

[llrs]

Another check that can be added as mentioned by @JosiahParry is to check that SystemRequirements include these elements: "Cargo (Rust's package manager), rustc", as according to CRAN's rust policy they are required. I don't know how often of if possible but maintainers might mention them in different order or after other system dependencies.

Also note that this would need to be done conditionally as other repositories, Bioconductor or others, might not be so specific.

[alexhroom]

@llrs I don't know whether that's "good enough": the original check contains this comment

        ## 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()

so the original author already considered SystemRequirements to not be robust enough

[llrs]

Yes @alexhroom I'm aware of that comment, but CRAN policy specifically says:

The package should declare

SystemRequirements: Cargo (Rust's package manager), rustc

Emphasis mine.

Even if it is not robust, if there aren't those two requirements, maintainers are not complying with CRAN's policy, so they might want to detect that. It might be easier to convince CRAN to check for something they themselves have decided than to try to come up with other ways to detect rust.

[alexhroom]

@llrs oh I misunderstood you: i thought you meant 'check' as in 'check that the package is compiling Rust code', not 'check for package CRAN compilance'! got it.

[aitap]

I agree this is true in most of the cases, but maybe there's a little chance that Cargo.toml is dynamically generated?

Note that this is happening after the package is compiled and installed in the temporary library. There may be an even smaller chance that the build process cleans up after itself and removes Cargo.toml. Some Rust-using packages on CRAN clean src/rust/vendor after the build (and before the make clean action) because its contents trip up other checks, but none go as far as removing src/rust altogether with Cargo.toml.

Of course a package could 2>&1 | grep -v -E '(cargo build| Compiling )' to work around the current check as well, but that would be pointless.

[JosiahParry]

@aitap you can also pass --quiet to the build process so that no messages appear at all. The use could also specify a specific version of Rust to use in the compilation by writing cargo +1.71 build for example. Or, they could just add a couple of spaces :)

[yutannihilation]

There may be an even smaller chance that the build process cleans up after itself and removes Cargo.toml

Ah, then, you are right, sorry. I just didn't know well about the build process.

Another counter example I just came up with is to use an unusual name of manifest file with --manifest-path. To be clear, I'm just saying this Cargo.toml solution might not be 100% accurate, and I don't know if it ultimately matters to the R core team. If you think this is acceptable to them, I think you can just propose the change.

(IMHO, if the R core wants accuracy, they should either make the SystemRequirements mandatory or add a new field that's more than "free-format, and only advisory" rather than polishing the heuristics.)

[llrs]

Nice addition with this last commit.

1. Maybe a comment explaining why these checks are added might help future readers.
2. Apologies for only raising this later than my initial comment: I haven't checked, if check_rust() is always run when R CMD check is used or only activated in a special check/branch/condition. I am not sure this is the right place to check for this, because maybe it would be run on Bioconductor or in other repositories checks and this could be against their policy.
   For example, I don't know how Bioconductor handles rust, I haven't seen any mention on their mailing list or slack. This could be well or break some packages there (and the repository systems would need to be updated/changed)

[llrs]

Typo?

Suggested change

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

[alexhroom]

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)

[llrs]

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.".

[aitap]

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