Skip to content

Improved Ubuntu CI #45

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Improved Ubuntu CI #45

wants to merge 2 commits into from

Conversation

lextm
Copy link

@lextm lextm commented Jan 8, 2021

  • Added Ubuntu 18.04 and 16.04.
  • Switched to Docker images, so later other distributions can be added similarly.
  • Modified Makefile to remove the quotes around PATH. This allows make to work on Ubuntu 18.04 and 16.04.

@knocte
Copy link
Member

knocte commented Jan 8, 2021

@lextm ubuntu1804 lane is failing

@lextm
Copy link
Author

lextm commented Jan 8, 2021

The failure was not caused by the changes, but instability around NuGet package restore. You can see the same changes work in https://github.com/lextm/dotdevelop/actions/runs/470535881

@knocte
Copy link
Member

knocte commented Jan 8, 2021

Well, you could squash the 2 commits into 1, to see if next CI run passes.

@lextm
Copy link
Author

lextm commented Jan 8, 2021

I intentionally left two commits, as that illustrated clearly why Makefile needs to be modified.

Two GitHub Actions runs in my fork can be used as reference,

@knocte
Copy link
Member

knocte commented Jan 8, 2021

My suggestion is that if you push with your --force in your branch, the CI will run again and that ugly red (x) will disappear. If you don't want to squash, you could just change the commit message of last commit slightly (e.g. with git commit --amend).

PS: Actually, thinking more about this, are you sure squashing the 2 commits into 1 is not the right approach? Because if you push the commits one by one, the first commit's CI would break, right?

@lextm
Copy link
Author

lextm commented Jan 8, 2021

The rule of GitHub Actions on pull request creation is that it only triggers a single run, no matter how many commits are there. That's why there was only one run for this pull request so far.

I know I can modify on my side to trigger another run (squash or whatever), but the repo owner can force rerun the workflow from Actions tab, which is a far more common way to resolve instable CI builds.

@knocte
Copy link
Member

knocte commented Jan 8, 2021

no matter how many commits are there

Not 100% accurate statement: it triggers a single run per push, so if you have 2 commits, you could push 1 first, and the 2nd later, in order to have CI run in each of them. I even did a script myself to push this way: https://github.com/nblockchain/fsx/blob/master/Tools/gitPush1by1.fsx

But even if you could not do the above, it's always preferable that CI is not broken in between commits, because it prevents bisections.

but the repo owner can force rerun the workflow from Actions tab, which is a far more common way to resolve instable CI builds.

Well, if you had proposed this PR from your fork, you wouldn't need the repo owner to help ;)

@lextm lextm changed the base branch from dotdevelop_oe_8.4.3.12_NoGit to dotdevelop_oe_8.6 January 13, 2021 17:41
@lextm
Copy link
Author

lextm commented Jan 13, 2021

Will rebase to 8.6 branch later today.

@lextm
Copy link
Author

lextm commented Jan 17, 2021

After rebase to 8.6, all checks now passed.

@knocte
Copy link
Member

knocte commented Jan 17, 2021

Can you do two pushes to check if 0e1dde1 also passes CI? (if it doesn't, then they should be squashed into 1)

@lytico
Copy link
Member

lytico commented Jan 19, 2021

@lextm
Copy link
Author

lextm commented Jan 19, 2021

@lytico I plan to add CentOS and Fedora in a separate pull request.

sudo apt-get update
sudo apt-get install -y sed build-essential intltool fsharp gtk-sharp2
sudo apt-get install -y software-properties-common
sudo add-apt-repository ppa:git-core/ppa -y
Copy link
Member

Choose a reason for hiding this comment

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

the 20.04 build was working already without the need to depend on a PPA; I guess the PPA is needed only for the older ubuntu versions? if so, it's better to not add in this CI lane

Copy link
Member

Choose a reason for hiding this comment

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

@lextm confirmed, please remove the ppa line from the 20.04 build

@knocte
Copy link
Member

knocte commented Jan 19, 2021

Can you do two pushes to check if 0e1dde1 also passes CI? (if it doesn't, then they should be squashed into 1)

FYI I've copied your branch to my fork, but doing one push per commit, and the first commit breaks CI:
https://github.com/knocte/monodevelop/commits/ubuntu-ci

So this is proof that, strictly speaking, the commits should be squashed into one before merging (or when merging). Having all commits pass CI is a good practice, to easean the work of future developers when bisecting problems.

@lytico lytico changed the base branch from dotdevelop_oe_8.6 to main January 19, 2021 15:07
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.

3 participants