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

Add "Repository" entry to keep {renv} happy #851

Closed
wants to merge 1 commit into from

Conversation

andrewheiss
Copy link

@andrewheiss andrewheiss commented Sep 12, 2023

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Add a repository entry to DESCRIPTION so that renv knows that this comes from a non-CRAN repository (addresses #850)

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Andrew Heiss

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@jgabry
Copy link
Member

jgabry commented Sep 12, 2023

Great, thanks for the PR!

@rok-cesnovar @andrjohns I can't think of any reason why this would cause any issues, and it would resolve a spurious warning from renv, so I think we should do it. But before I merge this, just wanted to check if you know of any reason why we shouldn't do it.

@rok-cesnovar
Copy link
Member

All for it.

@jgabry
Copy link
Member

jgabry commented Sep 12, 2023

Also, @andrewheiss don't worry about the failing unit tests. Those are presumably due to #843 and should get sorted out when that's merged.

@andrewheiss
Copy link
Author

Ok good, I won't :) Everything tested fine on my local fork on macOS, but I haven't updated to 2.33 yet

@kevinushey
Copy link

I don't think you want to add it directly to the source package here; it should only be added when the package is built, if the built package is going to be uploaded to the public repository.

Otherwise, tools that install this package from GitHub (e.g. remotes::install_github()) could get confused when installing directly from this repository.

@kevinushey
Copy link

Perhaps more succinctly: I think the binary packages that are built and published at https://github.com/stan-dev/r-packages should include Repository: Stan or something similar, and that should happen in some sort of pre-build step for the packages that are built and published there.

@andrewheiss
Copy link
Author

Ahh that makes sense. In that case, disregard this PR @jgabry. The Repository: Stan line will need to be added somewhere else (maybe here? https://github.com/stan-dev/r-packages/blob/gh-pages/src/contrib/PACKAGES and maybe added to the other packages that are hosted at https://mc-stan.org/r-packages/ like rstan, rstanarm, and StanHeaders - idk how Stan's package building infrastructure works 😕)

@jgabry
Copy link
Member

jgabry commented Sep 13, 2023

Thanks for the info @kevinushey!

Ahh that makes sense. In that case, disregard this PR @jgabry. The Repository: Stan line will need to be added somewhere else (maybe here? https://github.com/stan-dev/r-packages/blob/gh-pages/src/contrib/PACKAGES and maybe added to the other packages that are hosted at https://mc-stan.org/r-packages/ like rstan, rstanarm, and StanHeaders - idk how Stan's package building infrastructure works 😕)

Yeah we can look into adding this only when the package is added to the repository for distributing the packages and not in the package source code itself. I'll close this PR but leave the issue open. Basically we use https://mc-stan.org/r-packages/ (which is just a drat repo) to host cmdstanr (not on CRAN) and then for experimental versions of packages that have a stable version on CRAN (e.g. preview versions of rstan or rstanarm, etc.).

@jgabry jgabry closed this Sep 13, 2023
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.

4 participants