r-lib / pkgbuild

Find tools needed to build R packages
https://pkgbuild.r-lib.org
Other
65 stars 33 forks source link

Can needs_compilation detect changes on Rust files? #115

Closed yutannihilation closed 1 year ago

yutannihilation commented 3 years ago

Currently, sources() consider only the files that ends with .c* or .f. May I send a pull request to add .rs here? Or, can this provide more general mechanism (e.g. via options()) that allows users to specify the files to watch on?

https://github.com/r-lib/pkgbuild/blob/d9717aad6ff0534846b36a827e8444174dc83a1a/R/compile-dll.R#L115

If this is possible, our workflow to develop R packages with Rust code becomes much easier. c.f. https://github.com/extendr/rextendr/issues/56

gaborcsardi commented 2 years ago

Do you still need this? Would this allow you to use pkgload::load_all()? Or you need it for another reason?

yutannihilation commented 2 years ago

Do you still need this? Would this allow you to use pkgload::load_all()?

We ended up implementing another version of devtools::document(), it might be less important now. But, I still believe it's nicer if we can customize pkgload::load_all(), which should be generally useful for other compile languages.

gaborcsardi commented 2 years ago

OK, do you need anything else apart from the file name extensions? I can add *.rs files, and also a mechanism to define extra files the DLL depends on.

yutannihilation commented 2 years ago

Thanks. From a perspective of a Rust user, I'd like to detect the changes on Cargo.toml, which defines the configurations of the Rust project (e.g. dependencies, compilation flags), in addition to *.rs. But, if this would make the implementation complex, only *.rs should be fine.

gaborcsardi commented 2 years ago

Is Cargo.toml in src as well? I guess R CMD check will not let you put it in /?

I am thinking about adding support for a Config/ field in DESCRIPTION that would let you define the sources of the dll, something like this, with comma separated globs:

Config/pkgbuild/dll-sources: Cargo.toml, *.rs

This would be probably in addition to the current *.c* and *.f, for simplicity.

yutannihilation commented 2 years ago

Is Cargo.toml in src as well?

Yes, this is the typical structure of an R packages using extendr:

.
├── R
│   └── extendr-wrappers.R
...
└── src
    ├── Makevars
    ├── Makevars.win
    ├── entrypoint.c
    └── rust
        ├── Cargo.toml
        └── src
            └── lib.rs

Config/ field in DESCRIPTION

Sounds good to me!

gaborcsardi commented 1 year ago

@yutannihilation I am sorry this has taken so long. I think I'll skip the config option in DESCRIPTION and will just also detect /src/**/Cargo.toml and /src/**/*.rs files. Is this still OK?

yutannihilation commented 1 year ago

I think so, thanks for addressing this!