Skip to content
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

Fully delegate package installation to rstantools #26

Merged
merged 7 commits into from
Mar 10, 2023
Merged

Fully delegate package installation to rstantools #26

merged 7 commits into from
Mar 10, 2023

Conversation

andrjohns
Copy link
Contributor

With the latest release of rstantools (2.3.0), we now support exposing stan functions during the installation process (so you no longer have to manually create the c++ sources).

This PR updates your package structure to fully delegate the installation/configuration to rstantools, allowing your package to remain compatible with future versions of rstan/StanHeaders.

I've also added a GHA R-CMD-CHECK workflow from the r-lib/actions repository so you can verify that all still works & passes

@jtimonen jtimonen merged commit 9885a1f into jtimonen:develop Mar 10, 2023
@jtimonen
Copy link
Owner

Thanks!

@jtimonen
Copy link
Owner

So the installation process now seems to generate an src/ directory, R/RcppExports.R and R/stanmodels.R inside the source location. So should I just .gitignore those?

@andrjohns
Copy link
Contributor Author

Yep that would be best

@andrjohns
Copy link
Contributor Author

When you're able, would you mind submitting a new release to CRAN with this PR?

@jtimonen
Copy link
Owner

I tried it, but got a bunch of warnings and notes that I don't get locally with R CMD check --as-cran. Some of them are easy to fix but some I might need help with.

@jtimonen
Copy link
Owner

I don't know how to fix this:

* checking whether package 'lgpr' can be installed ... WARNING
Found the following significant warnings:
  WARNING: The tools required to build C++ code for R were not found.

It is now the only warning that I get on winbuilder with r-devel

@andrjohns
Copy link
Contributor Author

That's not a warning that you need to fix. It's referring to the system being used to run the checks, rather than the configuration of your package itself

@jtimonen
Copy link
Owner

But I also get this on my own mac, even though my c++ toolchain should be okay and the installation works, so I don't really understand what is going on.

@andrjohns
Copy link
Contributor Author

The check/warning has no relation to your package or its configuration, just the system itself. If you receive it on your system, but the tools are installed, then it's just a false positive.

@jtimonen
Copy link
Owner

Okay thanks, I have sent a message to CRAN explaining this.

@andrjohns
Copy link
Contributor Author

@jtimonen It looks like this warning might have rstantools-related after all (apologies for the misdirection!), since it's also being seen in another package using the same framework.

Did CRAN have a response to your message? I'm trying to replicate locally (no luck yet), but was hoping that they might have responded with more context/information

@jtimonen
Copy link
Owner

No they haven't responded

@andrjohns
Copy link
Contributor Author

Sorry again for the wrong advice! The warning occurs when calling rstan::expose_functions() with rstan 2.21 and only when rstudio isn't being used, which was why this was missed initially.

I've submitted a PR to rstantools to fix the warning, and I'll let you know when it's on CRAN

@andrjohns
Copy link
Contributor Author

rstantools 2.3.1 has been released and now the warning should no longer pop up

@andrjohns
Copy link
Contributor Author

@jtimonen sorry to double-ping, but would you mind trying the submission again soon?

@jtimonen
Copy link
Owner

jtimonen commented Apr 6, 2023

Done

@andrjohns
Copy link
Contributor Author

Done

Thanks, very much appreciated!

@jtimonen
Copy link
Owner

jtimonen commented Apr 6, 2023

https://cran.r-project.org/web/packages/lgpr/index.html <- now on CRAN (binaries not yet updated)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants