ropensci-review-tools / pkgcheck

Check whether a package is ready for submission to rOpenSci's peer-review system
https://docs.ropensci.org/pkgcheck/
18 stars 6 forks source link

github workflow for others to run pkgcheck #62

Closed mpadge closed 2 years ago

mpadge commented 3 years ago

Directly inspired by riskmetric discussion, develop a workflow for others to put in their own repos and run pkgcheck on GitHub runners. One of the biggest challenges would be working out an appropriate venue to push and host the HTML results. Current workflow in roreviewapi only works because it is the bot making the calls, and the bot has admin privileges. It would likely be necessary for individual use to push to a static/pkgcheck directory on gh-pages branch, and to instruct users beforehand to switch on gh-pages. All could be checked in workflow anyway, and only pushed + served if branch exists. Ping @dgkf

maelle commented 3 years ago

You could host the results as Markdown as "output object" from a check run https://docs.github.com/en/rest/reference/checks + https://docs.github.com/en/github/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks#checks

assignUser commented 2 years ago

With {touchstone} we comment the benchmark results on the PR but I assume that people are not running {pkgcheck} on each PR right? Rather it would be a manual action triggered during preparations for submitting to rOpensci? Or do you want to provide a "pkgcheck bot" that would emulate the @ropensci-review-bot check package function?

mpadge commented 2 years ago

It will be a "standard" workflow like current actions/check-package calling rcmdcheck::rcmdcheck(), but will instead call pkgcheck::pkgcheck(), and will fail any time a check gets a :heavy_multiplication_x: sign. The bot is already up and running, but not publicly accessible - that might change down the line, so people can use our exact system for checks, but setting up a GHA is first priority.

assignUser commented 2 years ago

So it would only fail on the first checks that are marked with either :heavy_check_mark: or :heavy_multiplication_x: and only output the other results (like goodpractice)?

assignUser commented 2 years ago

@mpadge First draft: https://github.com/assignUser/pkgcheck-action Example run: https://github.com/assignUser/simstudy/runs/4833853207?check_suite_focus=true#step:2:2222

assignUser commented 2 years ago

With correctly rendered markdown: https://github.com/assignUser/simstudy/runs/4834000068?check_suite_focus=true#step:2:2237

mpadge commented 2 years ago

@assignUser That's fantastic! The only issue i see is that universal-ctags on ubuntu is not recent enough to work properly. My current work-around is pkgstats::ctags_install(), which also (by default) requires sudo = TRUE. Or the steps there could be hard-coded like in the pkgstats workflow. I could also set up a PPA with a more recent version than current apt, but that wouldn't speed up whole workflow that much.

Other than that, the output seems spot on, and it's great that you've even re-mapped with visjs path to upload the artefact! Feel free to start at PR here at any time, please :smile:

assignUser commented 2 years ago

Glad you like it @mpadge ! I am still not sure what the issue is with Ubuntu and ctags (as I don't see any errors but I believe you xD), would switching to a MacOS runner help?

I could just install ctags/global from source if it is important, if that takes to long maybe we change to a docker based action. I would have to look into that though.

Keeping the action in a separate repo has the advantage that you can publish it on GitHub marketplace but if you want to keep it together with the package that works too, we do it for {touchstone}.

assignUser commented 2 years ago

ctags_install seems to work :) https://github.com/assignUser/simstudy/runs/4834215814?check_suite_focus=true#step:2:2174

mpadge commented 2 years ago

Thanks @assignUser. The ubuntu-ctags issue is because there are multiple forks of ctags itself, most of which are pinned to version 5.9, so just using --version is of no use. pkgstats requires universal-ctags 5.9. The Ubuntu apt hosts both:

The version needed for pkgstats is universal 5.9, so has to be compiled. Because of version confusion between all of these different forks, the only test for required functionality is via the internal function, pkgstats::ctags_test().


More importantly: Having the action in a separate repo - and on the marketplace - is definitely the idea here, so people can easily use it via a single line in their own actions files. The action could also easily build directly on top of the pkgcheck docker container which includes the latest universal-ctags. That container is built with this action, so could easily be used in your pkgcheck-action repo. Here's the link to the GH docs on containers in a workflow file, which aren't so clear. I'm pretty sure it should now work by just including:

jobs:
  my_job:
    runs-on: ubuntu-latest
    container: mpadge/pkgcheck

That one extra line should then run the workflow in that container with both global and universal-ctags 5.9 installed.

assignUser commented 2 years ago

Here is a run based on your docker image: https://github.com/assignUser/simstudy/runs/4841332260?check_suite_focus=true Works fine :) Currently this does require a dedicated workflow, I am currently looking into creating a "real" docker based action.

Should I publish the action to the marketplace or do you want to transfer the repo to your org?

mpadge commented 2 years ago

If it would be okay with you, it would be much easier (= less confusing) for people to use it if it was part of this org. You would of course be clearly identifiable as the creator of the repo. It would then also be really useful to put a helper function within this repo - you could then author that too, and then you'd be added as an author to this package as well, and the main README would then be able to directly point to the action and recommend usage. Would you work for you?

assignUser commented 2 years ago

I agree that it would be better to have the actions be published under your org, to make clear that it is "official".

What you described works for me :) a helper function to setup the workflow or what were you thinking of?

mpadge commented 2 years ago

What you described works for me :)

:heavy_check_mark: :+1: :rocket:

a helper function to setup the workflow ... ?

:heavy_check_mark: yep, exactly.

The easiest way would be for you to give me admin on the repo so i can transfer it - you'd then automatically retain admin there. Would that be okay? Once this is all up and running, it would be a sufficient advance that we at rOpenSci might want to announce it via a Tech Note, of which you would then also be an author. That might take a while, and maybe could be done as part of a general announcement to coincide with a CRAN release of pkgcheck.

assignUser commented 2 years ago

Sounds good, I added you :tada: I am still fiddeling around with using an actual one-line docker action, too see if the increased maintenance effort off maintaining the container etc. is worth it. For that I am building a container and pushing it to ghcr (which would maybe be a good idea for the pkgcheck container so it will be available in the repo?)

assignUser commented 2 years ago

The pure docker version works and takes only half the time and does not need a dedicated workflow but can be added to any other workflow: https://github.com/assignUser/simstudy/actions/runs/1709854246

I am not sure if the more involved maintenance is worth it, what do you think @mpadge ?

mpadge commented 2 years ago

This package has to be very actively maintained regardless, because it is used to deliver the automatic responses from our ropensci-review-bot (like here). The more options we can give to people to help them prepare packages in advance, the easier the entire review process is likely to be. So extra maintenance here is generally worthwhile. In this case, I imagine there could well be people who prefer a dedicated pkgcheck action inserted into their repo via a helper function, while others might prefer just inserting a single extra uses: pkgcheck-action line. All of which means the answer to your question is, "yes" - i do think the extra maintenance is worthwhile.

assignUser commented 2 years ago

After some insidious whitespace problems the action should now be ready to go, including post to issue functionality. https://github.com/ropensci-review-tools/pkgcheck-action

The only thing left as I see it is a proper read me which I started to draft in the readme branch and the pkgcheck::use_github_check function.

Also marketplace release but I am not sure if that works with rolling releases.

mpadge commented 2 years ago

This is all supremely awesome Jacob, thank you so much!! I'll check it all out via a test repo early next week and report back then. Great work!