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

Github Actions CI workflow #250

Merged
merged 11 commits into from
Sep 23, 2020
Merged

Github Actions CI workflow #250

merged 11 commits into from
Sep 23, 2020

Conversation

drodin
Copy link
Member

@drodin drodin commented Aug 22, 2020

Just a proposal for switching current testing from Travis/AppVeyor to GitHub Actions.

Work example:

On the plus side:

  • lower barrier for new packages contributions
  • no need for a separate testing repo
  • you can see right in the PR which toolchains are failing

On the negative side:

  • harder per-package customization of test scripts
  • ??? please tell me

Most of the workflow file mimics Travis/AppVeyour scripts. The hacky stuff:

  • Action runs on: pushing to pr.* branch or pull request to the main branch, in both cases it is checked that at least one file in the cmake/projects/ path was modified. I couldn't find a way to discard a run on pull request to the main branch based on PR's head branch name, though it is possible to check and fail later...
  • The PROJECT_DIR is extracted from the branch name, which MUST follow pr.package-name notation, it is possible, though more complicated, to extract it from changed files list
  • It is possible to test build for multiple projects if changes are in one push or pull request
  • Default matrix is constructed in .github/workflows/set_matrix.py by substituting "example" field in .github/workflows/ci/matrix.json according to changed files

Not in the current commit:

  • For packages that need additional system packages it is possible to read and install them from a list file in package dir
  • For packages like Boost which use more than one example dir there are different possibilities, depending on wether or not a clean environment is needed for each example build

Customization of matrix and build scripts (this shouldn't be done for 90% of packages):

  • Default matrix can be overridden by cmake/projects/<PROJECT_NAME>/ci/matrix.json
  • Default build scripts can be overridden by "script" property of the custom matrix

Would like to hear your thoughts and proposals.
Thank you.

@drodin
Copy link
Member Author

drodin commented Aug 22, 2020

@bkotzz As this PR is alternative to #29 #9 I would like to know, why this development was stopped?
@rbsheth Would also like to hear your comments on that matter.
Thank you.

@NeroBurner
Copy link

NeroBurner commented Aug 22, 2020

This would compile ALL projects for each commit/PR

Can this be limited to only changed projects? Or only the documentation if that is updated

@drodin
Copy link
Member Author

drodin commented Aug 22, 2020

@NeroBurner This will not compile all projects, see example.
In this proof of concept the updated project is determined from a branch name - pr.project-name
This is of course not ideal as convention on branch naming and atomicity of a commit must be enforced.
It is possible to determine project-name
from the list of updated files.

@NeroBurner
Copy link

Ist there in github actions something like rules:changes in gitlab?

https://docs.gitlab.com/ee/ci/yaml/#ruleschanges

@drodin
Copy link
Member Author

drodin commented Aug 23, 2020

@drodin
Copy link
Member Author

drodin commented Aug 24, 2020

There is a change in GitHub Actions which I was not aware of. It is possible to read matrix from json, which can be an output of another job. This led to a possibility of easy customization of build scripts. See previous 2 commits.

@rbsheth
Copy link
Member

rbsheth commented Aug 24, 2020

@bkotzz As this PR is alternative to #29 #9 I would like to know, why this development was stopped?
@rbsheth Would also like to hear your comments on that matter.
Thank you.

Hi @drodin, as per #9 this is something we've been wanting for a long time. @bkotzz started work on #29 a while ago but work stopped due to other priorities. Of course we would like to make adding/updating packages easier and removing the need for a separate testing repo makes life a lot easier.

Running the tests per updated project is exactly what we need. The matrix override is perfect since we need to log when certain packages have failing toolchains.

I would like to have an easy way to port/copy over the old travis/appveyor YAMLs into the new matrix format so that we can effectively delete hunter-testing. Perhaps we can do that after this PR is merged. No more pkg.template and pr.pkg.package-name!

Another nice to have would be to build the documentation in Github actions too, so we can remove our Travis dependency.

This is really great stuff, thank you for doing it!

@drodin
Copy link
Member Author

drodin commented Aug 26, 2020

See 4d3bb34 for docs testing, it's a separate workflow, which runs on push/pull to master branch if any file in docs/** or examples/** has been changed (not sure if I had to include other paths here) example:
bad docs: https://github.com/drodin-tests/hunter/runs/1030556427?check_suite_focus=true
good docs: https://github.com/drodin-tests/hunter/runs/1030560970?check_suite_focus=true

@rbsheth
Copy link
Member

rbsheth commented Aug 27, 2020

@drodin is this ready to merge? We have some scripts to port over the hunter-testing YAMLs to this format, but they need a little work. We also need to update the docs to reflect this change.

@drodin
Copy link
Member Author

drodin commented Aug 28, 2020

@rbsheth It's not ideal, see below. Though it's non-intrusive, so can be merged for live testing.

These problems arise from a few projects which have components (f.e. Boost)

  • No project testing will be done if ONLY examples folder was modified, as there is no definitive way to determine project name and load it's matrix override, if present
  • Everything is defined in a matrix, so if f.e. project's author wants to modify just a build script (to mere install missing system package), default matrix needs to be copied to projects ci subfolder, this will lead to old toolchain definitions in overriding matrix (it's however the same with the current test system). Alternative would be to remove 'script' property from matrix and just call override build scripts from project ci subfolder, if present. But this will remove the possibility to define different test scripts for different components examples.
  • Other alternatives with separate or more complex matrices are possible, if you have ideas please share them.

If commenting out a toolchain in a matrix serves only the purpose of determination which toolchains are failing there are better ways to log this:

ps: With 621c69c it's possible to manually start a test for any project from GitHub interface or through API call.
pps: As for the docs, I can only make a draft, which had to be checked by native English speaker

@rbsheth
Copy link
Member

rbsheth commented Aug 28, 2020

I think we should support a set number of toolchains, and possibly deprecate older ones as new toolchains are adopted. The fact that toolchains can get outdated in the current scheme is a flaw not a feature IMO.

Are there cases where we want to install new system packages for a package? I think that should be discouraged, and in the worst case can be added to the top level build script.

As for only modifying the examples/ directory, I think we can manually run a test in that edge case. I can't remember the last time that happened.

Does the boost build work with all its components?

@craffael
Copy link

Are there cases where we want to install new system packages for a package? I think that should be discouraged, and in the worst case can be added to the top level build script.

An example is the OCCT package which needs the mesa-common-dev system package. Another example is the OpenCV package which needs also two additional system packages (libegl1-mesa-dev and libgl1-mesa-dev)...

@drodin
Copy link
Member Author

drodin commented Aug 29, 2020

@rbsheth
I also think it's a flaw. But even if we'll go with monolith matrix for GitHub Actions, it will be much easy, compared to the current testing model, to find toolchain definitions in project override matrices and replace them with newer default toolchains, and rerun tests.

Well ok this was not the best example ;) Another example - to define a system env variable, you'll have to copy and override matrix and the build script, changing one line (see Boost example)

Ok.

I've made an example test matrix override for Boost based on current travis/appveyour scripts - drodin-tests@3de5b97. You can see a test run here https://github.com/drodin-tests/hunter/actions/runs/229629184 Took just a bit more than an hour.
Two failures: Boost-thread analyze-cxx17 (I think due to the clang 7 vs 9 version difference), and Boost-python vs-14-2015-win64-sdk-8-1 (this can be fixed, I just forgot to change python version in the matrix)

Btw, I had to add python version to the matrix, see 1232674

@drodin
Copy link
Member Author

drodin commented Sep 1, 2020

@rbsheth While there is always a room for improvement, I think we need to finalize it on this stage.

  • with fc898fc it is possible to override just a build script without touching the default matrix (the appropriate script build.sh/build.cmd should be placed in project's ci subfolder)

  • dd5498e is for setting the status of project testing on GitHub Pages. We can skip merging this commit if this functionality is not needed. For this to work GitHub Pages must be enabled in hunter repo (in the separate gh-pages orphan branch), drodin@0cf945d should be merged. This will look like this https://drodin-tests.github.io/hunter/

@drodin
Copy link
Member Author

drodin commented Sep 7, 2020

a10097c - is a documentation draft for GitHub Actions CI testing. I've took the liberty to decouple testing (including local) into separate section, with just one paragraph included in creating/updating section.

@drodin drodin changed the title WIP: Github Actions CI workflow Github Actions CI workflow Sep 7, 2020
@drodin drodin marked this pull request as ready for review September 7, 2020 04:52
@rbsheth
Copy link
Member

rbsheth commented Sep 9, 2020

Looking good! I will review this week and try to convert the older projects too.

@drodin
Copy link
Member Author

drodin commented Sep 9, 2020

@rbsheth I've updated a Boost example ci override a bit, may be of help for converting older projects - see drodin@01de433

@rbsheth
Copy link
Member

rbsheth commented Sep 23, 2020

@drodin Thanks for the updates, I'm going to try merging this now. Let's see what happens!

@rbsheth rbsheth merged commit 9e86c5d into cpp-pm:master Sep 23, 2020
@rbsheth
Copy link
Member

rbsheth commented Sep 23, 2020

drodin@0cf945d cherry-picked into orphan branch gh-pages

@rbsheth
Copy link
Member

rbsheth commented Sep 23, 2020

gh-pages is broken for some reason, we'll need to look into it

@drodin
Copy link
Member Author

drodin commented Sep 23, 2020

@rbsheth pages should build correctly on the first tested packages, there is a sort of tested packages which is failing now
You should be able to test some package by manually running Actions>CI>Run Workflow>Enter package name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants