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

Merge all things in one workflow file, reducing duplication of CI code #17355

Merged
merged 44 commits into from
Aug 27, 2020

Conversation

ret2libc
Copy link
Contributor

@ret2libc ret2libc commented Jul 24, 2020

Reasons for this change

Whenever we want to test some new configuration, we may want to test it both on PRs and/or on the master branch. Moreover, some configurations may be tested only on releases. However, we also want to test some particular things on PRs with particular names. For example, the iOS package is built only on release, but if you are doing some changes related to that, you may want to test the iOS package built also in PR. The same is true for ASAN, static, osx, etc.

At the same time, when doing a release we want to ensure that all "regular" checks are performed and are green and after that performs additional operations (e.g. built iOS package, build windows installer, etc.)

Current state

Right now the only solution was to create multiple workflow files and duplicate the code needed to do the tests in both files. This may result in a lot of duplicated "devops" code, having the risk of testing things differently when on PR and when doing releases or commits on master branch.

Solution proposed in this PR

By merging everything in one single file we can avoid the duplication of the "devops" code and just choose when to run some particular jobs based on the kind of triggering event (e.g. some jobs may be run only on schedule, others only when doing a release, etc.). At the same time, it allows to create dependencies between jobs (e.g. the create-release can only run if all the various build and tests jobs on various configurations pass).

Possible issues of this PR

  • some jobs will be skipped, but they are going to be shown anyway in the list of jobs. For example, in this PR the job "Build on CentOS 6" will not run, but you will see anyway the entry below, though it will be marked as "skip".
  • on PRs, linux-acr-clang-build runs instead of linux-acr-clang-tests, but you will see anyway linux-acr-clang-tests (though nothing will be run there).

@ret2libc ret2libc requested a review from trufae as a code owner July 24, 2020 16:08
@ret2libc ret2libc marked this pull request as draft July 24, 2020 16:08
@ret2libc ret2libc removed the request for review from trufae July 24, 2020 16:08
@github-actions github-actions bot added the infrastructure Issues related to the radare2/cutter infrastructure label Jul 24, 2020
@XVilka
Copy link
Contributor

XVilka commented Jul 27, 2020

To be honest, I don't like this approach. While it reduces some code duplication the resulting file is massive and hard to navigate. Currently CI configuration neatly split allowing easier understanding and improvements.

@ret2libc
Copy link
Contributor Author

To be honest, I don't like this approach. While it reduces some code duplication the resulting file is massive and hard to navigate. Currently CI configuration neatly split allowing easier understanding and improvements.

You are right. I'm not very convinced by this either, though I've read that's the way github workflows are supposed to work right now :( But yeah, I agree it does not look ideal. The fact is that I would like to have things dependent on one another and this seems the only way.

To summarize, this is what I'm trying to achieve:

  • make release steps depend on build-and-test jobs (linux-acr-clang-test, linux-meson-gcc-test, etc.)
  • make sure releases steps do not work if some of the build steps do not work well.
  • reduce duplication of "devops" code
  • allow to run some steps of the release process (e.g. building cydia package or android) when a particular branch name is used. This would be useful to check changes to those things more easily.

I want to first see how this works, we can then choose what to do. I won't push too hard for this, I understand it has some pros and cons.

@ret2libc
Copy link
Contributor Author

BTW, this approach should allow us to easily perform things like #16535 without having duplicate yaml code around. Of course, it would be ideal if we could split the big yaml in smaller ones and "include" them in one file, but this is still not possible in github ci :(

@wargio
Copy link
Contributor

wargio commented Aug 6, 2020

looks ok, but why there are some cancelled jobs?

@ret2libc
Copy link
Contributor Author

ret2libc commented Aug 7, 2020

look ok, but why there are some cancelled jobs?

They are not cancelled, they are skipped. IMO github should just not show those skipped jobs at all in the list, but apparently it is not possible at the moment... :(

@XVilka
Copy link
Contributor

XVilka commented Aug 10, 2020

@thestr4ng3r @trufae are you ok with this change? Please take a look, otherwise lets merge it.

@ret2libc
Copy link
Contributor Author

Ok, conflicts solved.
I have tried to summarize, again, the reasons for this PR. See #17355 (comment)

In particular, this would have been useful for recent PRs done by @trufae related to sdk.sh, ios, static fixes.

@ret2libc ret2libc marked this pull request as draft August 26, 2020 08:42
@ret2libc ret2libc marked this pull request as ready for review August 26, 2020 08:45
@ret2libc
Copy link
Contributor Author

As an example, you can see #17525 and https://github.com/radareorg/radare2/actions/runs/225017795 . The PR was named improve-workflow-ios-osx-debian and indeed ios, osx and debian jobs ran.

@XVilka XVilka merged commit dfde84d into radareorg:master Aug 27, 2020
ret2libc added a commit to ret2libc/radare2 that referenced this pull request Aug 31, 2020
ret2libc added a commit to ret2libc/radare2 that referenced this pull request Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Issues related to the radare2/cutter infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants