extendr / rextendr

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

Adds CRAN compliance helpers and vignette #320

Closed JosiahParry closed 6 months ago

JosiahParry commented 9 months ago

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:

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

JosiahParry commented 9 months ago

@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 commented 9 months ago

Ok let me check

Ilia-Kosenkov commented 9 months ago

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

JosiahParry commented 9 months ago

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

Ilia-Kosenkov commented 9 months ago

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 commented 9 months ago

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

JosiahParry commented 9 months ago

@Ilia-Kosenkov true! But i don't think that's the core of the problem. If you look at the CI output nothing is showing up :O

image

I'll save it for another time.

Ilia-Kosenkov commented 9 months ago

Yep which means some sort of parsing had failed. I will check this out in an hour or so

Ilia-Kosenkov commented 9 months ago

The reason I insist on this is that I believe this flaky test shows instability in parsing, which may negatively affect users. So we better figure it out

Ilia-Kosenkov commented 9 months ago

@JosiahParry , can I push to your branch here?

JosiahParry commented 9 months ago

@Ilia-Kosenkov confirmed

JosiahParry commented 9 months ago

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

JosiahParry commented 8 months ago

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 commented 6 months ago

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

JosiahParry commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

JosiahParry commented 6 months ago

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

Ilia-Kosenkov commented 6 months ago

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

JosiahParry commented 6 months ago

No more changes from me at this time :)