search

extendr / rextendr

An R package that helps scaffolding extendr-enabled packages or compiling Rust code dynamically
https://extendr.github.io/rextendr/
Other
175 stars 25 forks source link

Release prepartation 0.3.0 #280

Closed Ilia-Kosenkov closed 1 year ago

Ilia-Kosenkov commented 1 year ago

Address small issues revealed while preparing for 0.3.0 release (#277).

Ilia-Kosenkov commented 1 year ago

@yutannihilation, it seems extendr is still using VECTOR_PTR, and recent PR to libR-sys broke stuff. Can you confirm? What is our plan then?

CGMossa commented 1 year ago

Well, extendr shouldn't. And in fact I'm actually sure that it doesn't use it in the sense that it doesn't do anything. It might be in there. It needs to be deleted anyways.

yutannihilation commented 1 year ago

it seems extendr is still using VECTOR_PTR,

As @CGMossa answered, it shouldn't because it's already removed by https://github.com/extendr/extendr/pull/512. So, if the test uses the GitHub version of extendr and the GitHub version of libR-sys, it shouldn't have any problem. Does the error mean it uses the release version of extendr with the GitHub version of libR-sys?

Ilia-Kosenkov commented 1 year ago

Ha, you are right, we are using dev version of libR-sys but crates version of extendr-api. https://github.com/extendr/rextendr/blob/f8f964ba268f4330188ddeb4255b1db4b676eedb/.github/workflows/R-CMD-check.yaml#L32

I guess I should remove this so we test against crates version. Or, on the contrary, test against all-dev versions.

yutannihilation commented 1 year ago

Oh, I see. I think rextendr should test against the crate version because it's probably what the most users want to use. But, it seems there's a reason to use the GitHub version... Can you pin the specific revision before the current HEAD for a workaround?

https://github.com/extendr/rextendr/pull/254

This probably means the new version of libR-sys and extendr should be released as soon as possible.

Ilia-Kosenkov commented 1 year ago

Ok, how about I test against dev for now, then we release libR-sys & extendr and we switch back to crates version? Here I needed dev version (I think) only to enable various feature tests like support for gated features & extendr attribute arguments.

And yes, we can pin whatever revision we want, since this env var contains basically toml representation of dependency.

JosiahParry commented 1 year ago

I don't know the intricacies of the CI system or how extendr really works, but my 2 cents is this: the CI should mimic exactly what is going to be available to the CRAN version of the package. If the CRAN version of the package will not use github dev versions of libR-sys & extender it should utilize the crates version. Otherwise there is a disconnect in expectations from what users will (likely) install

eitsupi commented 1 year ago

I recall from tests, I don't think this package is currently tested on CRAN at all. https://github.com/extendr/rextendr/blob/f8f964ba268f4330188ddeb4255b1db4b676eedb/tests/testthat.R#L8-L10

Ideally, tests that can also be run on CRAN should be modified to run on CRAN. (But I know this is work that necessarily has to be done at this release.)

Ilia-Kosenkov commented 1 year ago

Yep we do not run tests on CRAN (though we could run some there). Since CRAN seems to have added rust support in some form, I guess we could revisit this at some point in the future after this release.

Ilia-Kosenkov commented 1 year ago

Ready for re-review

Ilia-Kosenkov commented 1 year ago

Ok so given the time & a couple of approves, I will merge this and cautiously proceed with submission.