ARROW-18345: [R] Create a CRAN-specific packaging checklist that lives in the R package directory #14678
Conversation
|
|
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
The release management guide has moved (partially?) to the arrow docs: https://github.com/apache/arrow/blob/master/docs/source/developers/release.rst
Might also be a good place to add this too?
There was a problem hiding this comment.
Why maintain multiple lists? Could just move the content from the wiki to here and put a link from the wiki pointing here.
There was a problem hiding this comment.
I moved the high-level content from the wiki to the release.rst file (whereas PACKAGING.md contains the specifics of what file and which repo but not necessarily the "why").
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
That would be make sync-cpp but that happens within make release :)
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
For windows it should be possible to use rhub once the release has been published and the binaries uploaded to the artifactory. For mac we might be able to use a modified version with the custom formula added as we do it for the nightlies? We also do have 10.13 and M1 runners now so running mac rhub might not be necessary?
There was a problem hiding this comment.
I think it's a good sanity check to make sure the rhub box can install the package (unless it becomes too hard to do or takes so long that the job times out or something).
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
We have a docker compose service for this now: archery docker r-revdepcheck (which is not documented I think 😁 )
There was a problem hiding this comment.
Before the next release I'll open an issue about this and fix it...I was never able to get the job to run (on my Ubuntu box my version of docker-compose was too old and I got syntax errors; on M1 the images run very slowly because they're not built for arm).
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
This was usually done in a fork afaik but I think we should not do that if possible. One of the things that always happened was removing the badges from the README as the links were sometimes flakey and causing issues. We should just add that (should be doable with a bit of sed) to make release
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
They are now uploaded to the apache artifactory during the release process https://apache.jfrog.io/ui/repos/tree/General/arrow/r
|
@nealrichardson Is this vaguely correct? I filled in some details of bits that I thought might be part of the process that weren't explicitly mentioned in the confluence guide but I may have gotten them wrong! |
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Why maintain multiple lists? Could just move the content from the wiki to here and put a link from the wiki pointing here.
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Are these necessary? This isn't something we've done before. The release-verification crossbow job could do these. But we generally don't care about source builds on win/mac, and Linux is covered in regular CI.
There was a problem hiding this comment.
I don't mind doing these checks when they come up...I see it as the R version of the "release verification script" since that's not part of the official verification script. I think it's reasonable that at least one developer should be able to reproduce a clean check on each OS?
There was a problem hiding this comment.
Has the fact that we've not done this before on each OS led to headaches with past releases, or not been a problem? I'd rather we didn't make more work for ourselves if it's not necessary, but if it is needed then fair enough. If it's more for good practice, could we have it as an optional step?
There was a problem hiding this comment.
I'll remove it and pursue this differently...I think the main thing I want to do is "verify the release candidate for R". The right way to do that is to add these checks to the verification script.
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Do this earlier because we always remove badges from README before submitting. You'll want to run urlchecker after removing badges.
There was a problem hiding this comment.
In principle the CRAN branch should originate at the release commit, no? I added a urlchecker bullet (and a "remove the badges from the readme" bullet) here.
There was a problem hiding this comment.
You can originate from the release commit while it is an RC. git checkout apache-arrow-x.y.z && git checkout -b r-x.y.x
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Unfortunately I think you have to rely on our CI for these. The rhub jobs will time out and fail.
There was a problem hiding this comment.
I removed that bullet point.
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Lately Jeroen has been merging these before the vote closes if it looks like the RC will pass. The binaries are versioned now so it is safe for him to deploy them whenever. That means you can test win-builder and macbuilder before the vote closes and include that as part of manual release verification.
If they are merged before the vote passes, you still want to follow up to switch off of the RC URL, but that can happen in a separate PR after the fact.
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Mostly just glance to make sure it's still valid (a translation of the use_release_issue() bullet for devtools::build_readme(), since we don't use README.Rmd). I updated the text here to reflect that.
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
Please can we rephrase this? What is "full documentation"? Do I need to have read it before I do this, or not? Let's make it explicitly clear what the role of each is.
There was a problem hiding this comment.
Great point...I'll take a stab at something better.
Co-authored-by: Neal Richardson <[email protected]>
Co-authored-by: Neal Richardson <[email protected]>
r/PACKAGING.md
Outdated
There was a problem hiding this comment.
| - [ ] `urlchecker::url_check()` | |
| - [ ] Run `urlchecker::url_check()` on the R directory, at the release commit. Ignore any errors with badges as they will be removed in the CRAN release branch. Fix any other errors and then cherry pick them into the CRAN release branch. |
paleolimbot
force-pushed
the
r-packaging-checkilist
branch
from
659ab34 to
a58be68
Compare
|
(Sorry, I had to rebase. I think I got all of your edits though! Keep on making them!) |
r/PACKAGING.md
Outdated
| - [ ] Evaluate the status of any failing | ||
| [nightly tests and nightly packaging builds](https://lists.apache.org/[email protected]): these are run | ||
| against the master branch and not the release branch, so the results may be | ||
| slightly out of date. After the CRAN-specific release branch has been created, | ||
| we can run any or all tests again using a GitHub PR and crossbow. |
There was a problem hiding this comment.
I'm confused by this step - is this a new additional step? It's not release verification, as that's something else. Kou later made #14654 which basically does everything we need to do in this step. We need more info here.
There was a problem hiding this comment.
The only R job that PR ran is r-binary-packages...in order to submit to CRAN we need to make sure all the r-** jobs are passing (or understand that if they are not that it will not result in a CRAN rejection, such as with the valgrind and homebrew nightlies that have been failing forever and haven't affected our CRAN status). I'll find a way to phrase this better.
r/PACKAGING.md
Outdated
| - [ ] `make build` (copies Arrow C++ into tools/cpp, prunes some unnecessary | ||
| components, and runs `R CMD build`) | ||
| - [ ] `devtools::check("arrow_X.X.X.tar.gz")` locally | ||
| - [ ] Run reverse dependency checks |
There was a problem hiding this comment.
That's a bit up in the air. There is a crossbow job that will do hit; however, it times out if you try to run it on a PR. If you run it locally via archery docker run <I forget the exact name> then you need an up-to-date version of docker-compose which doesn't ship on Ubuntu yet (at least the last time I tried it) and it takes several hours. I did it last time using https://gist.github.com/paleolimbot/630fdab1e204d70fea97633d8fa15ccb , which takes ten minutes or so depending on how fast you can rebuild arrow. I have a feeling we can rewrite the crossbow version so that it also takes less time...then it can run nightly and we can just treat it like all the other tests.
I think I just petered out trying to fit that in a bullet point. Any suggestions?
There was a problem hiding this comment.
I have removed the crossbow job because it would have been a bunch of work to fix the timeout. But I was wondering why revdepcheck was taking SO LONG (even if you take the 2 core gha machines into account). And locally it will still timeout for a different reason ({targets} timesout with multiple workers??).
But I would be fine with reworking the job to use your gist or even the build in function I did not know exists: https://www.r-bloggers.com/2019/04/checking-reverse-dependencies-the-tiny-way/ current docs: https://stat.ethz.ch/R-manual/R-patched/library/tools/html/check_packages_in_dir.html
There was a problem hiding this comment.
Doing it locally is a pain as I'm having to install a whole load of system libraries I don't need, to be able to run the gist; please can we rework the crossbow job?
There was a problem hiding this comment.
Oh yeah, that gist is way better on MacOS because the binary packages for MacOS are self-sufficient.
Co-authored-by: Neal Richardson <[email protected]>
Co-authored-by: Neal Richardson <[email protected]>
Co-authored-by: Neal Richardson <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
Co-authored-by: Nic Crane <[email protected]>
|
Any more updates here, or can we merge? |
|
Let's merge this for now and make any further changes in a follow-up PR |
thisisnic
left a comment
thisisnic
left a comment
There was a problem hiding this comment.
Thanks for creating this @paleolimbot !
4 participants
A first stab at documenting the post-Arrow-release/pre-CRAN submission process! Builds on excellent documentation on the Confluence page ( https://cwiki.apache.org/confluence/display/ARROW/Release+Management+Guide#ReleaseManagementGuide-UpdatingRpackages ) but in a more "checklisty" form to make sure we don't miss steps.