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

Adds CRAN compliance helpers and vignette #320

Merged
merged 41 commits into from
Dec 29, 2023
Merged

Conversation

JosiahParry
Copy link
Contributor

This PR superceds #313. The purpose of this PR is create a pathforward for developers who wish to publish extendr packages on CRAN. It is based on prqlr and my experience publishing rsgeo to CRAN.

The PR adds:

  • use_cran_defaults() which adds configure and configure.win files, modifies Makevars and Makevars.win to use CRAN requirements
  • vendor_pkgs() which vendors crates appropriately
    • uses cargo generate-lockfile for brand new packages without a Cargo.lock file
    • automatically updates vendor-config.toml
  • vignettes/articles/cran-compliance.Rmd which describes the process for making R packages CRAN compliant.

It does not add functionality to document crate authors. rsgeo does not do this and has not been problematic in getting the package published—though it may in the future, I am unsure.

Nonetheless, I used these functions to publish rsgeo and it has been successful. This PR provides minimal funcitonality to provide a path forward towards CRAN compliant packages in an opt-in manner.

Follow up tasks

R/cran-compliance.R Outdated Show resolved Hide resolved
R/cran-compliance.R Outdated Show resolved Hide resolved
R/cran-compliance.R Outdated Show resolved Hide resolved
R/cran-compliance.R Outdated Show resolved Hide resolved
R/cran-compliance.R Outdated Show resolved Hide resolved
vignettes/articles/cran-compliance.Rmd Outdated Show resolved Hide resolved
vignettes/articles/cran-compliance.Rmd Outdated Show resolved Hide resolved
vignettes/articles/cran-compliance.Rmd Outdated Show resolved Hide resolved
vignettes/articles/cran-compliance.Rmd Outdated Show resolved Hide resolved
vignettes/articles/cran-compliance.Rmd Outdated Show resolved Hide resolved
@JosiahParry
Copy link
Contributor Author

@Ilia-Kosenkov I cannot figure out what the error is with these snapshot tests for testing vendoring output. @CGMossa was able to pull the branch, run the tests, and they all passed. Do you have any idea what might be causing the failure? I'm tempted to remove to test all together to appease the CI gods.

@Ilia-Kosenkov
Copy link
Member

Ok let me check

@Ilia-Kosenkov
Copy link
Member

I got this snapshot failure -- you incorrectly parsed stdout
image

@JosiahParry
Copy link
Contributor Author

Which is yet different than the CI errors 🙈 ! Thank you for testing

R/cran-compliance.R Outdated Show resolved Hide resolved
@Ilia-Kosenkov
Copy link
Member

Ilia-Kosenkov commented Sep 24, 2023

Maybe you can extract vendored packages (if I udnerstand your logic) using this regex?
Vendoring\s([a-z0-9_][a-z0-9_-]*?)(?=\s)\s(.+?)(?=\s)

@Ilia-Kosenkov
Copy link
Member

You changed snapshot to snapshot x, but the actual snapshot still has print(x), hence it complains :)
image

@JosiahParry
Copy link
Contributor Author

@Ilia-Kosenkov your changes have fixed CI! I've added a note to NEWS.md as well.

@JosiahParry
Copy link
Contributor Author

I'd like to revisit this PR. I used this fork to publish {arcpbf} to CRAN. Using these functions has made it very easy to publish extendr packages to CRAN. There was no back and forth about the use of Rust in this package, no back and forth about the build or compilation process etc.

I think making it easy to publish extendr packages to CRAN is the best way to make this project widely useful and interesting.

Please let me know what is needed to merge this PR.

@Ilia-Kosenkov
Copy link
Member

Hey @JosiahParry , what' the status of this PR?

@JosiahParry
Copy link
Contributor Author

Please let me know what is needed to merge this PR

What do you need from me to merge this PR? I've been using it myself for cran publications.

@Ilia-Kosenkov
Copy link
Member

Tests are failing due to version mismatch, can you pull it locally and run tests - it should tell you which snapshot is wrong.

@Ilia-Kosenkov
Copy link
Member

And add a reference to your Pr in News.md, I think it is missing

@JosiahParry
Copy link
Contributor Author

Cool sounds good! I'll appease PR gods tomorrow-ish and update news to get this through!

@Ilia-Kosenkov
Copy link
Member

Looks solid! If you have no more changes to make @JosiahParry , should we merge it?

@JosiahParry
Copy link
Contributor Author

No more changes from me at this time :)

@Ilia-Kosenkov Ilia-Kosenkov merged commit edc5476 into extendr:main Dec 29, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants