search

lcolladotor / biocthis

Automate package and project setup for Bioconductor packages
https://lcolladotor.github.io/biocthis/
49 stars 16 forks source link

add template file for GHA #11

Closed LiNk-NY closed 3 years ago

LiNk-NY commented 4 years ago

@lcolladotor This will create a working check-bioc.yml in the .github/workflows directory based on the user's Bioconductor version and R version. Windows and Mac tests are not supported yet.

LiNk-NY commented 3 years ago

Hi Leo, @lcolladotor This should add multiple platform support. Let me know what you think. Best, Marcel

FelixErnst commented 3 years ago

Hi Marcel @LiNk-NY, Hi Leonardo @lcolladotor,

I setup GHA for new project and I came across the issue, that I need GHA to work with devel on master and with release on another branch due to changes in S4Vectors (the renaming of vertical_slot_names to parallel_slot_names).

Wouldn't it make sense, to suggest the GHA to be active by default for a branch != master if the version is release? Bioconductor expects development to happen on master, so maybe this warrants a little bit of nudging towards avoiding a potential problem. Maybe this can be solved with an interactive choice.

Let me know, what you think. Thanks.

Felix

LiNk-NY commented 3 years ago

Hi Felix, @FelixErnst This should work on both release and devel. I will update the documentation soon to provide more guidance. If you're on Bioc-release, you'd run this the same way and push to your RELEASE_3_11 branch. You'd have two 'check-bioc.yaml' files, one on the release branch and another in the master branch.

FelixErnst commented 3 years ago

Hi Marcel,

I didn't want to suggest the GHA not working 😄 . I am sorry if that's how it came across.

The suggestion is mostly about the first 3 lines in the GHA template file, which don't restrict the GHA to a certain branch by default.

For example, if devel is specified as version this wouldn't make too much sense:

on:
  push:
    - RELEASE_3_11
  pull_request:
    - RELEASE_3_11

Vice verse if release is specified, this is also problematic in some cases within half a year:

on:
  push:
    - master
  pull_request:
    - master

Also leaving the on argument empty works at the beginning, but within a release cycle, this may change.

use_bioc_github_action currently doesn't take any arguments, but maybe an optional branch argument could help populate the restrictions and make the user aware of bioconductor expecting the master branch to work with devel.

LiNk-NY commented 3 years ago

Hi Felix, @FelixErnst

I didn't want to suggest the GHA not working . I am sorry if that's how it came across.

No worries, I didn't interpret it that way. I was trying to say that it is designed to work based on the user's current Bioconductor installation. That is, if you are running Bioc-devel, the file that is generated would be for devel.

The suggestion is mostly about the first 3 lines in the GHA template file, which don't restrict the GHA to a certain branch by default.

For example, if devel is specified as version this wouldn't make too much sense:

on:
  push:
    - RELEASE_3_11
  pull_request:
    - RELEASE_3_11

Vice verse if release is specified, this is also problematic in some cases within half a year:

on:
  push:
    - master
  pull_request:
    - master

AFAICR, this is not necessary when you have a yaml file in each branch. I could be wrong though. I haven't tested this given that the builds were broken yesterday.

Also leaving the on argument empty works at the beginning, but within a release cycle, this may change.

The code is dependent on BiocManager so it should work for every release cycle.

use_bioc_github_action currently doesn't take any arguments, but maybe an optional branch argument could help populate the restrictions and make the user aware of bioconductor expecting the master branch to work with devel.

See point above. The normal workflow is to have the master branch point to the Bioc devel branch. Note that the user can always edit their file after it is generated by the function.

FelixErnst commented 3 years ago

Ok maybe using it will tell us, how it behaves during two release cylces.

The code is dependent on BiocManager so it should work for every release cycle.

My guess is (As you said we couldn't test it yesterday), that once a package has a RELEASE branch, the GHA config will be applied as defined for all branches. At that point a new devel version is available, which is not necessarily compatible with the on before the RELEASE branch was created,

In that scenario, using bioc-devel as defined in the GHA leads to potential check errors on the RELEASE branches, if the user does not intervene, because devel before RELEASE is fine and devel after RELEASE is not fine per se. The time point when the GHA is run makes the difference.

See point above. The normal workflow is to have the master branch point to the Bioc devel branch. Note that the user can always edit their file after it is generated by the function.

Of course, but adding a GHA for the master branch only, will lead to expected behavior on the master branch and will solve future issues on RELEASE branches by not being run on those.

So I would add at the limit to the master branch in GHA template by default. Otherwise users might "complain" after half a year. With the limit they might "complain" as well, but not because of an error, but because they need help to apply GHA to a different branch.

But hey, this is really a minor issue. Thanks for adding the template!

LiNk-NY commented 3 years ago

Hi Felix! @FelixErnst

My guess is (As you said we couldn't test it yesterday), that once a package has a RELEASE branch, the GHA config will be applied as defined for all branches. At that point a new devel version is available, which is not necessarily compatible with the on before the RELEASE branch was created,

Have a look at this release branch package with a GitHub Action: https://github.com/waldronlab/cBioPortalData/blob/RELEASE_3_11/.github/workflows/main.yml A GHA takes precedence if present on that branch. It only gets run when pushes are made to the RELEASE_3_11 branch. When a maintainer creates a release branch on GitHub, they should run the use_bioc_github_action function on that branch. Other branches are expected to be development branches so I think it would be advantageous to have the checks run the GHA for other branches.

In that scenario, using bioc-devel as defined in the GHA leads to potential check errors on the RELEASE branches, if the user does not intervene, because devel before RELEASE is fine and devel after RELEASE is not fine per se. The time point when the GHA is run makes the difference.

There is no version mix up because the GHA will be using the appropriate Docker container or BiocManager command. As long as containers are up to date, that should not be a problem.

lcolladotor commented 3 years ago

Wow, this is a really fancy PR!

LiNk-NY commented 3 years ago

Thanks Leo! I'm trying to do the package justice :wink:

lcolladotor commented 3 years ago

Hi! This is an older discussion on the GHA forum that was partly why I ended up having the code duplication for docker then mac & windows https://github.community/t/run-matrix-job-on-macos-and-on-ubuntu-in-container/16359.

Having said that, the template could have common pieces of code and re-use the code that is duplicated.

lcolladotor commented 3 years ago

I've been working on this for a while, but well, I'm a stuck. I got your version to work, then started adding the features that biocthis has. And it nearly works. The issue I'm having is that I can't get GHA to let me specify volumes: /home/runner/work/_temp/Library:/usr/local/lib/R/host-site-library without this crashing the Windows and macOS runs. If I don't use that, then I can't restore the R packages cache on Linux running bioconductor's docker image (that's the status at https://github.com/lcolladotor/testmatrix/commit/10484be0575fd3715f54fdc41b10d2c4ccd23c3e).

lcolladotor commented 3 years ago

At https://github.com/lcolladotor/testmatrix/commit/285cfbe4dc277fd71e9de5ada77b2eed8dff65a1 (aka https://github.com/lcolladotor/testmatrix/blob/285cfbe4dc277fd71e9de5ada77b2eed8dff65a1/.github/workflows/check-bioc.yml) I got the cache to work with bioc-docker and my test was successful ^^ https://github.com/lcolladotor/testmatrix/runs/1192083901?check_suite_focus=true#step:9:22

I'll merge your PR tomorrow then =)

I'm still thinking about the part of setting the versions vs having the GHA workflow detect them from the git branch when you make a git push. Your approach is likely better though it involves a few more commits (every time bioc-devel changes, to update the R version) which I know is actually just once per year. I guess that also invites users to download the latest GHA workflow on biocthis and at least compare against it, in case it has new features, etc.

Anyway, this will be a major change since it reduces nearly all the code duplication (the R cache part is duplicated right now, though that can be solved with the config matrix if the temp directory path is consistent on Windows; that's where the R user library lives; I'll check more logs tomorrow).

Thanks again!