-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Can we generate extendr-wrappers.R at the same time of compiling Rust code? #56
Comments
Apparently, it cannot because But, fn main() {
let metadata = helloextendr::get_helloextendr_metadata();
print!(
"{}",
metadata.make_r_wrappers(true, "helloextendr").unwrap()
);
} full commit: yutannihilation/helloextendr@e556d81 |
@clauswilke @andy-thomason @Ilia-Kosenkov |
Yes, your Also, have you tested your method on Windows? Didn't we have problems with |
I don't think this is just my preference. This matters because if this works the current steps: # Compile the Rust code (If you are using RStudio, just run "Install and Restart")
devtools::install()
# Re-generate wrappers
rextendr::register_extendr()
# Re-generate documentation and NAMESPACE (If you are using RStudio, just run "Document")
devtools::document() can be reduced to just one step (currently, devtools::document() The problem with |
Ah, just remembered the problem with |
One other issue to think about: Do we have potential problems with the path where the |
One last issue: We have had discussions about more complex wrapper code, see here: One other thought: Things are a bit awkward with the current |
Would like to get @dfalbel's perspective also. |
I don't think there's any problem as it's simple as just redirecting stdout to
Sorry, I don't get the point here. Why does the use of
Sorry, I don't understand this point either... In either way, the wrapper code is generated on Rust's side. The difference is how
No, I'd say they are different in that I would emphasize this point. The current rextendr's workflow has a chicken-egg problem; if If we want "the equivalent for |
If you follow the instructions in the first section of this post, for example, I don't think the wrappers would end up in the right location: https://kbroman.org/pkg_primer/pages/build.html
We're talking about more wrapper code that doesn't exist yet. It's not a given that it will be generated on the Rust side. There is no reason that it has to, I believe. |
Yes, I agree with you here. So, are you arguing that it might be easier to tweak the generated R code programmatically if we do it in |
Yes, I've never been that excited about having compiled Rust code spit out R code. I'm aware of the chicken-and-egg problem and I don't like it but I also think we shouldn't parse Rust code in R if we can avoid it. Do you know why I think more generally we should take a bigger picture perspective and not worry so much about how things work with the current |
I think this is the explanation for why the package gets compiled: https://roxygen2.r-lib.org/reference/load.html |
Okay, I think I'm getting your point...
I think it's here, which is called inside |
We could certainly write a function I think all of this comes down to mostly writing a new load function that does the following three steps:
|
I typed out my comment while you typed yours. Yes, modifying |
Actually, not so fast. I think |
Ah, true. Hmm... |
Maybe it's my long-term use of LaTeX, but I'm totally comfortable with keeping it at this for now: devtools::install(quick = TRUE)
rextendr::register_extendr()
devtools::document() And rextendr::register_extendr(force_wrappers = TRUE)
devtools::install(quick = TRUE)
rextendr::register_extendr()
devtools::document() Then, the next step might be to implement |
Hmm, okay. I feel I'll probably end up generating the wrappers in |
Reflect |
I'm not fully convinced, but I also feel I probably cannot convince other members with my idea. I'm closing this and will file a new one for |
I'm going to reopen this issue. I'm now quite convinced that there is no reliable way that an R function can build a package, load the dynamic library, and then call a function from it. This sequence of steps is only reliable if R gets restarted in-between, and that's not something an R function can (or should) trigger. Without restart, there's always the risk that the shared library doesn't get updated. So, we need to look for other alternatives. Building the wrappers during compile is attractive. However, I can also now better formulate what was bothering me about the proposed approach: I don't think a compile should overwrite the R package sources. That's just too much magic. Therefore, how about the following: What if the compile just generates the wrappers and leaves them in |
Thinking about this issue again, I now agree with this part. Yet another alternative might be to call |
I thought about it. In theory it's possible to generate the |
To be clear, I think it doesn't need to be c.f. https://doc.rust-lang.org/cargo/guide/project-layout.html#package-layout
|
Will you implement this? |
It's unlikely I'd get to it this week. So if you have time and want to give it a try please go ahead. |
Sure, I'll try a PR. |
We explored this option on #63, but we found it doesn't look nicer than |
As described on helloextendr's README, the current steps to produce the wrapper are:
But, I come to wonder if explicit
rextendr::register_extendr()
is really needed. Can we generate it on compilation by tweaking somebuild.rs
? (OrMakevars
?)The text was updated successfully, but these errors were encountered: