Closed Ilia-Kosenkov closed 1 year ago
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..
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.
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 https://github.com/extendr/rextendr/issues/239#issuecomment-1436690432, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIDVSGMZC675H77A5BD5B3WYNAA7ANCNFSM6AAAAAAVBU73IQ . You are receiving this because you commented.Message ID: @.***>
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.
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 https://github.com/extendr/rextendr/issues/239#issuecomment-1437049890, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIDVSGDLCXYOAFHI2X7FC3WYNYTJANCNFSM6AAAAAAVBU73IQ . You are receiving this because you commented.Message ID: @.***>
Turns out it was me who incorrectly added features
parameter (#140). I guess it is up to me to fix it :)
See this https://github.com/extendr/extendr/issues/481#issuecomment-1435708706 issue for reference.
Currently, any arguments passed to
features
simply produce a new[feature]
block inCargo.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
asfeatures = "ndarray"
enablingextendr-api/ndarray
.Proposed solutions:
features
completely and rely onextendr_deps = list(`extendr-api` = list(version = "*", features = array("ndarray", 1)))
, which is a mouthful, and even if documented properly, will be misusedfeatures
but instead we go intoextendr_deps
, findextendr-api
and addfeatures
to an existing list of features (or create a new one). This should be a reliable thing that works even ifextendr_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 keeprextendr
in sync with any features introduced inextendr
, which is a reasonable tradeoff for configuration safety.