rust_source()'s features argument seems to be misleading

[Ilia-Kosenkov]

See this extendr/extendr#481 (comment) issue for reference.

Currently, any arguments passed to features simply produce a new [feature] block in Cargo.toml. While it may be useful (?) to some extent, we are talking here about dynamic compilation, which is unlikely to be a complex project.

Consumers, however, may treat features as features = "ndarray" enabling extendr-api/ndarray.

Proposed solutions:

• We drop features completely and rely on
  extendr_deps = list(`extendr-api` = list(version = "*", features = array("ndarray", 1))), which is a mouthful, and even if documented properly, will be misused
• We keep features but instead we go into extendr_deps, find extendr-api and add features to an existing list of features (or create a new one). This should be a reliable thing that works even if extendr_deps is provided by the user.

Personally I prefer the latter, since in the majority of cases it will just simplify all the code to rextendr::rust_source(code = code, features = c("ndarray", "graphics")). If we want to allow only selected features, we would have to keep rextendr in sync with any features introduced in extendr, which is a reasonable tradeoff for configuration safety.

[CGMossa]

I barely understood the issue here, but I'm all for a solution, that repurposes already written files, in order to minimise recompilation. It is not ideal that a [feature] block is created all the time.
I also think that your prefered approach resonates with me.
Although people (and me) love to have an equivalence between toml-format and what is passed in R-functions... So good that you bring that up.
A little complication is necessary though, as I think of extendR as needing to be seamless integration with Rust, and not just convenient integration..

[Ilia-Kosenkov]

I may be wrong since I do not recall why and how we added this features parameter. The issue is we pass it directly to toml while consumers (me including) expect that it is just a set of features that can be enabled for extendr-api.
I am not even sure at this point that we need to allow writing to Cargo.toml directly.

It feels like we need an extendr_features = c("ndarray", "serde", "graphics", "either", "whatever") and a use_dev_extendr = TRUE parameters which will cover like all of the issues we get from people.

And another parameter for rust_function, something like extendr_attributes = list(use_try_from = TRUE). And that's it, all covered.

[CGMossa]

I agree. The issue is compilation-time more than anything (to me). For instance `serde` (with derive-feature) is costly on compilation. But that's interesting perspective. `rextendr` should have opinionated defaults, so that people can use it out-of-the-box;

…

On Mon, Feb 20, 2023 at 11:16 AM Ilia Kosenkov ***@***.***> wrote: I may be wrong since I do not recall why and how we added this features parameter. The issue is we pass it directly to toml while consumers (me including) expect that it is just a set of features that can be enabled for extendr-api. I am not even sure at this point that we need to allow writing to Cargo.toml directly. It feels like we need an extendr_features = c("ndarray", "serde", "graphics", "either", "whatever") and a use_dev_extendr = TRUE parameters which will cover like all of the issues we get from people. And another parameter for rust_function, something like extendr_attributes = list(use_try_from = TRUE). And that's it, all covered. — Reply to this email directly, view it on GitHub <#239 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIDVSGMZC675H77A5BD5B3WYNAA7ANCNFSM6AAAAAAVBU73IQ> . You are receiving this because you commented.Message ID: ***@***.***>

[multimeric]

Hmm, shouldn't enabling the ndarray feature on extendr also install the package as a dependency, making dependencies = list(ndarray = "*") redundant?

Still, it's not like we're providing a general serialization method even now, since the structure you pass into rust_source doesn't correspond to a cargo.toml. So I'm happy to make changes to make it even more user friendly.

[CGMossa]

Unless extendr reexports ndarray, then no, you still need `ndarray` as a dependency if you are to use it in the intermediate crate.

…

On Mon, Feb 20, 2023 at 2:46 PM Michael Milton ***@***.***> wrote: Hmm, shouldn't enabling the ndarray feature on extendr also install the package as a dependency, making dependencies = list(ndarray = "*") redundant? Still, it's not like we're providing a general serialization method even now, since the structure you pass into rust_source doesn't correspond to a cargo.toml. So I'm happy to make changes to make it even more user friendly. — Reply to this email directly, view it on GitHub <#239 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIDVSGDLCXYOAFHI2X7FC3WYNYTJANCNFSM6AAAAAAVBU73IQ> . You are receiving this because you commented.Message ID: ***@***.***>

[Ilia-Kosenkov]

Turns out it was me who incorrectly added features parameter (#140). I guess it is up to me to fix it :)