extendr / rextendr

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

Test `use_extendr()` on CI #54

Closed Ilia-Kosenkov closed 3 years ago

Ilia-Kosenkov commented 3 years ago

In light of #53, I suggest adding a separate step that creates an empty package and adds extendr dependencies. If we add a simple test (expect_equal(hello_world(), "Hello world!")), then R CMD check will reveal any potential problems with extendr setup.

yutannihilation commented 3 years ago

Sounds good. I think we can use usethis::create_package() to create an empty package.

https://usethis.r-lib.org/reference/create_package.html

Ilia-Kosenkov commented 3 years ago

I have an example here https://www.github.com/Ilia-Kosenkov/rextendr/tree/use-extendr-ci-test/.github%2Fworkflows%2FR-CMD-check.yaml

yutannihilation commented 3 years ago

Thanks, I have some questions.

I think we should do this outside of the repository.

d <- tempfile()
dir.create(d)
setwd(d)

Shouldn't this be done in use_extendr()?

usethis::use_build_ignore(".*\\.o$", escape = FALSE)

What's the intent of these two devtools::document()? I'm wondering if it should be like this instead:

devtools::install()
rextendr::register_extendr()
devtools::document()
Ilia-Kosenkov commented 3 years ago

@yutannihilation, I made a quick prototype. I am not sure about buildignore - devtools::document builds Rust source so package contains also build artifacts, like .o and .dll.

First devtools::document builds source and produces R code. Second call updates NAMESPACES and *rd docs.

If your solution is better, I have no objections.

yutannihilation commented 3 years ago

I made a quick prototype. I am not sure about buildignore - devtools::document builds Rust source so package contains also build artifacts, like .o and .dll.

Ah, I see. Sorry, I was a bit confused.

First devtools::document builds source and produces R code.

I mean, if I understand correctly, it doesn't. You need to call rextendr::register_extendr() explicitly to produce R code, and it needs the package to be installed to let .Call() find the symbol wrap__make_<package>_wrappers.

Ilia-Kosenkov commented 3 years ago

@yutannihilation, Then I am lost as this script works fine in my local environment. We need to test both approaches.

yutannihilation commented 3 years ago

Yes, it works, but I think it's because use_extendr() creates R/extendr-wrappers.R, not that the compiled rust code.

(Sorry, I didn't follow the discussions around the wrapper generation, so I might be wrong...)

Ilia-Kosenkov commented 3 years ago

@yutannihilation, I also do not understand fully at what point wrapper generation happens. However,

``` devtools::install() rextendr::register_extendr() devtools::document() ``` does not work for me -- last `document()` call complains about no function `wrap__make_testpkg_wrappers`. Maybe I am doing something wrong. Double `devtools::document` works fine though.
Ilia-Kosenkov commented 3 years ago

Here is the last working example: https://github.com/Ilia-Kosenkov/rextendr/runs/2046343796?check_suite_focus=true

yutannihilation commented 3 years ago

Double devtools::document works fine though.

What do you mean by "works" here? devtools::document() doesn't generate R/extendr-wrappers.R (e.g. if you tweak the roxygen strings on lib.rs, you'll notice it won't be reflected to R/extendr-wrappers.R). So, if we are testing "whether the default files created by rextendr::use_extendr() is correct," it's fine. But, if we want to test if the wrapper generation from Rust code is correct, devtools::document() is not enough.

But, sorry, the steps I proposed was incorrect... It seems we definitely need double devtools::document() AND installing and executing rextendr::register_extendr(). So, maybe the following?

# Generate NAMESPACE from the default wrapper created by use_extendr()
devtools::document()

# Install the package so that register_extendr() can find `wrap__make_testpkg_wrappers`.
devtools::install()

# Re-generate R/extendr-wrappers.R
rextendr::register_extendr()

# Re-generate documentation
devtools::document()
Ilia-Kosenkov commented 3 years ago

@yutannihilation, I will test your example later today.

yutannihilation commented 3 years ago

Thanks (and sorry for taking your time), but I'm rethinking the mechanism to generate the wrapper R code, so could you wait for a while?

https://github.com/extendr/rextendr/issues/56

clauswilke commented 3 years ago

Just to confirm: With the current setup, this is the correct sequence if you want to test both use_extendr() and register_extendr():

rextendr::use_extendr()
devtools::document()
devtools::install()
rextendr::register_extendr()
devtools::document()
yutannihilation commented 3 years ago

@Ilia-Kosenkov Sorry for stopping you, now that #56 reached a conclusion, at least at the moment, that we should stick with the steps @clauswilke described here, we can start implementing the test.

Ilia-Kosenkov commented 3 years ago

@yutannihilation, No problems, I opened a PR, we can discuss test setup there.

yutannihilation commented 3 years ago

Is this already solved by #61?

Ilia-Kosenkov commented 3 years ago

I missed the latest comment here. Yes, I believe it was solved in #61 and then improved afterward.

yutannihilation commented 3 years ago

Thanks for confirming, let's close this.