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

WIP: Enable a github actions build #1415

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

cwendling
Copy link
Member

No description provided.

@cwendling cwendling force-pushed the github-action branch 4 times, most recently from 99c729a to 9c09e27 Compare November 7, 2023 17:44
@cwendling
Copy link
Member Author

OK, so. @mate-desktop/core-team do we want something like this? I hear that we have Travis credits issues, so maybe we can unload some of this to GitHub Actions? I don't know much of anything about it, but it seems to work, and seems to allow a lot of things.
I'm not advertising anything, but had a fairly good experience with using it in Universal-CTags and Geany (I did not setup it there).

@lukefromdc
Copy link
Member

lukefromdc commented Nov 7, 2023 via email

@raveit65
Copy link
Member

raveit65 commented Nov 7, 2023

From which travis-CI issues did you heard exactly?
For me everything is working.
With travis-CI we also do releases and upload them to our web-site, we display ccpcheck results to gh-pages, and it works for stable and master branch with CI builds for rpm- and deb-based distros.
Do you really want to create a such configuration for all repositories and dismiss a working CI configuration?
We can merge this because it exists, but i don't want spend any spare time for an ihmo unnecessary CI rewrite for the whole organization.
But if all other want to switch....go ahead.

@raveit65
Copy link
Member

raveit65 commented Nov 7, 2023

Btw. i reduced the travis CI build time about 50% 2 weeks ago.

@cwendling
Copy link
Member Author

From which travis-CI issues did you heard exactly?

Build credits, I think it was you mentioning we're needing to ask for renewal all the time… I'm happy if it's not an issue (anymore) though.

For me everything is working.

For me as well, minus the mentioned credits issue, which I might not be up-to-date about.

With travis-CI we also do releases and upload them to our web-site, we display ccpcheck results to gh-pages, and it works for stable and master branch with CI builds for rpm- and deb-based distros.

Yeah I know it's complex, and this here is just a first attempt in case it can be useful.

Do you really want to create a such configuration for all repositories and dismiss a working CI configuration?

No, I don't. But I certainly would even less want CI to stop working one day out of the blue because we run out of credits or alike. But again, if you tell me I'm mistaken and it's not an issue, I just lost a bit of time and it's alright.

We can merge this because it exists, but i don't want spend any spare time for an ihmo unnecessary CI rewrite for the whole organization.

Only if it has actual value, because maintenance has a cost. Feel free to dump this, I'm not emotional about it 😄

@raveit65
Copy link
Member

raveit65 commented Nov 8, 2023

Build credits, I think it was you mentioning we're needing to ask for renewal all the time… I'm happy if it's not an issue (anymore) though.

I will explain it again , (hopefully last time)

  • We are sponsored in general by travis as open-source projekt.
  • Every time they gave us 25000 credits.
  • When our credits begin running low again a person from the team needs to write a simple one liner mail and ask again for 25000 credits
  • Usually after 1 hour credits are updated.

It is understandable that travis-Ci wants to control the sponsored build time. I don't think that is an issue unless the team is too lazy to write an email with one sentence.
I could reduce builds for every push with disabling build pushed pull request. Now we use only build pushed branches.
That means we use only 2 builds (fedora+debian) for every push, before it was 4 builds.
For example when you do a force push 15 times in a PR it will trigger 30 builds of the code.
It is always possible to temporarily disable travis-Ci to avoid this excessive usage in a PR.
grafik
https://app.travis-ci.com/github/mate-desktop/mate-panel/settings

We can merge this because it exists, but i don't want spend any spare time for an ihmo unnecessary CI rewrite for the whole organization.

Only if it has actual value, because maintenance has a cost. Feel free to dump this, I'm not emotional about it 😄

No, only you can dump your ideas. I don't do that.

Any way, beside from my concerns and the question who like to do this rewrite for every repository,
is the github-runner really for free and without any limits when it is used for 48 repositories?
I wrote a CI configuration for gitlab (with releasing und uploading to a download-server) and the gitlab-runner isn't free.

Maybe it make sense to replace debian build from travis-ci with a github-actions build to reduce build time again,
but i recommend not to replace fedora build as it it use for several other things.
I don't want to fall back in a time where we create releases by hand.

I agree with you not to use ccpscheck builds.... who needs this?
debian:testing docker tag was used because stable debian do not use current stable Mate packages.
For ubuntu i would select ubuntu:rolling tag which use newest and stable package releases. When using LTS you can run in the same problem as with debian. See https://hub.docker.com/_/ubuntu/

PS: I disabled travis-CI temporarily for mate-panel in case you want to work on this PR.

@cwendling
Copy link
Member Author

It is understandable that travis-Ci wants to control the sponsored build time. I don't think that is an issue unless the team is too lazy to write an email with one sentence.

It's (probably) OK as long as we have somebody

  • noticing
  • knowing the procedure. Is it written down anywhere, and can non-core people do it? (e.g. if we all disappear one day…)

I could reduce builds for every push with disabling build pushed pull request.

Well, that's kind of one of my concerns: we're disabling things because we can't afford them. No, we shouldn't waste resources, but we shouldn't be enticed to lower QA either.

Now we use only build pushed branches.

IIUC, this means that we don't run CI on pull requests made outside of the mate-desktop repository, right? This is an issue IMO, as it limits the checks we do on non-member PRs.

That means we use only 2 builds (fedora+debian) for every push, before it was 4 builds.
For example when you do a force push 15 times in a PR it will trigger 30 builds of the code.
It is always possible to temporarily disable travis-Ci to avoid this excessive usage in a PR.

One other option, if I understand the settings correctly, could be to keep building PRs, but either

  • only build pushes to master (if we can configure that)
  • stop pushing PR branches to the main repo and do that on personal forks instead.

is the github-runner really for free and without any limits when it is used for 48 repositories?

It (currently) is free for public repositories: "GitHub Actions usage is free for standard GitHub-hosted runners in public repositories, and for self-hosted runners." However, we of course can't be sure what the future holds.

Maybe it make sense to replace debian build from travis-ci with a github-actions build to reduce build time again,
but i recommend not to replace fedora build as it it use for several other things.

Well, if it makes sense to unload part of it only, sure.

I agree with you not to use ccpscheck builds.... who needs this?

Did I say anything about disabling it? Admittedly I don't use it, but I have just moved it to a separate job in order to run it concurrently, and have the results easier to reach. If nobody uses it maybe we can remove it though, as it's never a blocking check, so the only value is if people actually look at the results.

debian:testing docker tag was used because stable debian do not use current stable Mate packages.
For ubuntu i would select ubuntu:rolling tag which use newest and stable package releases. When using LTS you can run in the same problem as with debian. See https://hub.docker.com/_/ubuntu/

For now I used a plain runner without adding a docker layer, and selected "ubuntu-latest", which currently is Ubuntu 22.04.3. But docker images should be fairly easy to use as well, universal-ctags seems to make use of a lot of them.

PS: I disabled travis-CI temporarily for mate-panel in case you want to work on this PR.

You shouldn't have to do this I think, I specifically made it on my fork because I remembered you telling me we didn't build those. And I indeed don't see Travis in the checks list.

@cwendling
Copy link
Member Author

BTW: I will not be pursuing this unless it's something the team desires. As @raveit65 suggests, getting this working for all repositories is a significant undertaking, and I don't find this fun enough to do it if nobody else cares.
I care about CI health in general, not which runners use it or anything.

@cwendling
Copy link
Member Author

One last thing to think about, regarding CI in general: it's useful for CI to be fast, so developers don't get frustrated or distracted. If you have CI results within a minute or two, it makes development faster in case an issue is found: the author might not have switched to another task, or not went to sleep, or whatever. I'm not saying that our CI is too slow for convenience ATM (it's pretty OK), but that making it faster (even if it means shorter jobs with a little more overall overhead) can be beneficial.

Personal experience with CI results within a minute was a lot nicer than another experience where results would generally take an hour or more :) Admittedly the hour-long ones caught more issues in general (because of a huge project I was less familiar with), which didn't help frustration at waiting times, but it also meant that by the time the results came in I was doing something entirely different, and sometimes even went to sleep 'til next morning.

Just some thoughts.

@raveit65
Copy link
Member

raveit65 commented Nov 8, 2023

It is understandable that travis-Ci wants to control the sponsored build time. I don't think that is an issue unless the team is too lazy to write an email with one sentence.

It's (probably) OK as long as we have somebody

* noticing

* knowing the procedure.  Is it written down anywhere, and can non-core people do it? (e.g. if we all disappear one day…)

It is written here and in several other reports. Writing docs is a big job, volunteers are wanted....

I could reduce builds for every push with disabling build pushed pull request.

Well, that's kind of one of my concerns: we're disabling things because we can't afford them. No, we shouldn't waste resources, but we shouldn't be enticed to lower QA either.

No, this doesn't have any impact, build pushed pull request means a build between PR- and master-branch will be triggered.

Now we use only build pushed branches.

IIUC, this means that we don't run CI on pull requests made outside of the mate-desktop repository, right? This is an issue IMO, as it limits the checks we do on non-member PRs.

No, build pushed branches means a build will be triggered for every push on every branch including PR-branches.

That means we use only 2 builds (fedora+debian) for every push, before it was 4 builds.
For example when you do a force push 15 times in a PR it will trigger 30 builds of the code.
It is always possible to temporarily disable travis-Ci to avoid this excessive usage in a PR.

One other option, if I understand the settings correctly, could be to keep building PRs, but either

* only build pushes to master (if we can configure that)

* stop pushing PR branches to the main repo and do that on personal forks instead.

There is only one setting possible for master/stable and PR branches possible as far as i know from travis docs.

is the github-runner really for free and without any limits when it is used for 48 repositories?

It (currently) is free for public repositories: "GitHub Actions usage is free for standard GitHub-hosted runners in public repositories, and for self-hosted runners." However, we of course can't be sure what the future holds.

Maybe it make sense to replace debian build from travis-ci with a github-actions build to reduce build time again,
but i recommend not to replace fedora build as it it use for several other things.

Well, if it makes sense to unload part of it only, sure.

I agree with you not to use ccpscheck builds.... who needs this?

Did I say anything about disabling it? Admittedly I don't use it, but I have just moved it to a separate job in order to run it concurrently, and have the results easier to reach. If nobody uses it maybe we can remove it though, as it's never a blocking check, so the only value is if people actually look at the results.

I read your comment in commit :-)
Do we need the real build for cppcheck run? I don't think so
https://github.com/mate-desktop/mate-panel/pull/1415/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R129

debian:testing docker tag was used because stable debian do not use current stable Mate packages.
For ubuntu i would select ubuntu:rolling tag which use newest and stable package releases. When using LTS you can run in the same problem as with debian. See https://hub.docker.com/_/ubuntu/

For now I used a plain runner without adding a docker layer, and selected "ubuntu-latest", which currently is Ubuntu 22.04.3. But docker images should be fairly easy to use as well, universal-ctags seems to make use of a lot of them.

As i said it is possible that LTS releases do not use latest stable Mate releases after we made a new major release and than you need to build a mate dependency from git and not from docker image repositories.
Also, it make sense to see build issues with new updated dependencies in CI.
...only a hint not a concern :-)

PS: I disabled travis-CI temporarily for mate-panel in case you want to work on this PR.

You shouldn't have to do this I think,

Why you want want to test a github-CI configuration with travis-CI?
They are no code changes in mate-panel. You only need to see that your CI configuration works with github-CI in this case.
It only eats sponsored build time.
Travis can be enabled again before merging when PR is ready.

I specifically made it on my fork because I remembered you telling me we didn't build those. And I indeed don't see Travis in the checks list.

??? Which checklist.
For seeing the travis site from screenshot you need a travis account. Mandatory for Mate developers as long we use travis-ci :-)

Edit: Now i see what you mean.
Travis builds are not in checklist anymore because travis is disabled.
It was used before.

@cwendling
Copy link
Member Author

I read your comment in commit :-)
Do we need the real build for cppcheck run? I don't think so
https://github.com/mate-desktop/mate-panel/pull/1415/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R129

Oh :) What I meant there was wondering whether the cppcheck run does need to be run after a successful build, or running it on a plain git clone enough. I guessed it was OK with a plain git clone, just adding the library dependencies to help with external declarations. OTOH, maybe it does matter for generated files? Not sure.
But then again, if nobody looks at it, we could just drop it.

Why you want want to test a github-CI configuration with travis-CI?

I don't, I just meant that I didn't think it was necessary to do anything for Travis not to run on this PR (see below).

??? Which checklist.

Normally any check results (including Travis) show up in GitHub PR UI, both in the merge area "All checks have passed", and then "show all checks", or in the Checks tab.

No, this doesn't have any impact, build pushed pull request means a build between PR- and master-branch will be triggered.
[…]
No, build pushed branches means a build will be triggered for every push on every branch including PR-branches.

Are you sure? I don't see any build for e.g. this PR in https://app.travis-ci.com/github/mate-desktop/mate-panel/builds

From my understanding, this only works on the repo on which Travis is set up, e.g. for branches in mate-desktop/mate-panel, not any fork of it. So yes, it works for PR if we push the PR branch to the target repository, but not if we push it to a fork.

Here (push to fork), it lists the 2 GitHub Actions, no Travis. OTOH, on 1413 (push to repo branch) it shows Travis CI - Branch. And on an older push-to-fork, we only have Travis CI - Pull Request, whereas on a same-era push-to-repo we have both PR and Branch.

@raveit65
Copy link
Member

raveit65 commented Nov 9, 2023

Are you sure? I don't see any build for e.g. this PR in https://app.travis-ci.com/github/mate-desktop/mate-panel/builds

From my understanding, this only works on the repo on which Travis is set up, e.g. for branches in mate-desktop/mate-panel, not any fork of it. So yes, it works for PR if we push the PR branch to the target repository, but not if we push it to a fork.

Here (push to fork), it lists the 2 GitHub Actions, no Travis. OTOH, on 1413 (push to repo branch) it shows Travis CI - Branch. And on an older push-to-fork, we only have Travis CI - Pull Request, whereas on a same-era push-to-repo we have both PR and Branch.

Ohh, you're right, I tested this only with internal PR branches before i disabled build pushed branches, of course we need this for PRs from external contributors.
That means the only option is to exclude debian build from travis-ci to avoid 4 builds for every push at travis and include it to github-actions-ci.
So, this is a good reason for your PR :-) I saw you included debian build already.
I will start with reviewing and helping if needed.
Can you comment out debian in https://github.com/mate-desktop/mate-panel/blob/master/.travis.yml#L73 with an extra commit in this PR for testing please? I can't commit to your branch :-)
I will enable build pushed branches than. Or do you have access to the switch from my previous screenshot? I am not sure if all people from core-team can do that.

Maybe it make sense to include archlinux build to github-actions? Archlinux was part of the origin travis config and disabled when travis-ci wasn't free anymore a while ago. Build dependencies are listed in .build.yml.

@cwendling
Copy link
Member Author

Ohh, you're right, I tested this only with internal PR branches before i disabled build pushed branches, of course we need this for PRs from external contributors. That means the only option is to exclude debian build from travis-ci to avoid 4 builds for every push at travis

Well, not, there's an alternative: we could ask everybody (including core team) to create PRs on their fork, so only the Travis PR build runs on PRs. It requires all of us to play along, but that's another option.

and include it to github-actions-ci. So, this is a good reason for your PR :-)

I saw you included debian build already.

Yeah, I figured I could try and see how containers worked. Seemed easy enough, minus the fact that apparently we're root already, and sudo isn't available. Fine, but one has to know :)

I will start with reviewing and helping if needed.

Thanks!

Can you comment out debian in https://github.com/mate-desktop/mate-panel/blob/master/.travis.yml#L73 with an extra commit in this PR for testing please?

Sure, will do.

I can't commit to your branch :-)

Can't you? That's a bummer, I check the option allowing maintainers though…

I will enable build pushed branches than. Or do you have access to the switch from my previous screenshot? I am not sure if all people from core-team can do that.

I do have access. I'm not sure if we really should re-enable this right away, because it'd be better to either

  • tell @mate-desktop/core-team not to push PR branches to the main repo but work on a fork
  • and/or wait to move the Debian runner to GitHub Actions if we're going that route.

What do you think?

Maybe it make sense to include archlinux build to github-actions? Archlinux was part of the origin travis config and disabled when travis-ci wasn't free anymore a while ago. Build dependencies are listed in .build.yml.

Sure, IMO it makes sense to add any setup that adds value, and Arch is definitely both a fairly different setup, and a widely used one.

@raveit65
Copy link
Member

raveit65 commented Nov 9, 2023

I do have access. I'm not sure if we really should re-enable this right away, because it'd be better to either

* tell @mate-desktop/core-team not to push PR branches to the main repo but work on a fork

* and/or wait to move the Debian runner to GitHub Actions if we're going that route.

What do you think?

At the moment we are only 5 active members who commit, so using a forked branch might be a interim solution.
Or team member disable build pushed branches for their PRs, which might be simpler.
In general build on debian docker container should move to github-actions.

@cwendling
Copy link
Member Author

At the moment we are only 5 active members who commit, so using a forked branch might be a interim solution.

From my POV it doesn't have to be interim, and that's how I'm used to do things everywhere else. And normally GitHub even allows maintainers to push to PR branches in forks if the author allowed this in the PR (which is the default). So I'm not sure if there's any downside, minus perhaps a little more bandwidth usage as you'd push some more commits to your fork (as other's commits wouldn't be there already, so pushing your PR branch would sync this) -- but that should not be much, it's just the few commits, and it's entirely transparent otherwise.

Or team member disable build pushed branches for their PRs, which might be simpler.

Is it easy to do it per-branch? I'm afraid it's easy to forget as well, and probably would still run a first unwanted duplicate, wouldn't it?

@raveit65
Copy link
Member

raveit65 commented Nov 9, 2023

I enabled build pushed branches for mate-panel. I like to neither both travis and actions can run together.

@raveit65
Copy link
Member

raveit65 commented Nov 9, 2023

Travis-ci run nicely on this PR. I will disabled it again until this PR is ready go.
https://app.travis-ci.com/github/mate-desktop/mate-panel/builds/267140960

@cwendling
Copy link
Member Author

Maybe it make sense to include archlinux build to github-actions? Archlinux was part of the origin travis config and disabled when travis-ci wasn't free anymore a while ago. Build dependencies are listed in .build.yml.

Added, although I had a bit of a hard time with pacman, it works now.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@cwendling cwendling force-pushed the github-action branch 8 times, most recently from cc840d1 to 649764a Compare November 15, 2023 09:43
@cwendling cwendling mentioned this pull request Nov 15, 2023
@cwendling
Copy link
Member Author

OK so apart from whether or not we should run scan-build, I think everything that was raised has been addressed.

Cppcheck report has been improved a lot, but is quite slower now (5+ minutes, going from fastest to slowest); but I think the improved accuracy is worth it.

@raveit65
Copy link
Member

raveit65 commented Feb 1, 2024

@cwendling
Is this ready to go?

As i said before i can pretty well live without scanbuild.

Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lukefromdc
Copy link
Member

Any news on this one?

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.

4 participants