This repository has been archived by the owner. It is now read-only.

Implement support for gitlab CI #1539

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@craftyguy
Member

craftyguy commented Jun 5, 2018

This implements support for the gitlab CI system. Wiki, static analysis,
and build tests are implemented.

@craftyguy craftyguy requested review from ollieparanoid and MartijnBraam Jun 5, 2018

@craftyguy

This comment has been minimized.

@craftyguy craftyguy referenced this pull request Jun 5, 2018

Closed

Moving away from GitHub? #37

@ollieparanoid

That was fast, thank you very much @craftyguy!
Could you also make it run the testsuite (test/testcases_fast.sh --all)?

@ollieparanoid

This comment has been minimized.

Member

ollieparanoid commented Jun 12, 2018

@craftyguy: How's it going, do you think we can make the schedule?

@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 12, 2018

Oh wow I didn't realize CI had to be working by tomorrow.. At the moment, I don't think so, but I will have some more time to work on it this evening.

Here's the current status: everything works except the test_qemu_running_processes.py run by testcases_fast.sh. I'm having trouble creating a loop device in docker. Some theories:

  • It seems that the test in losetup.py that I wrote a few months ago (looking in /sys/modules/loop) may not be sufficient when running in a docker thing, since I can run 'losetup -f' in the container and get a loop device even though the loop module isn't loaded (according to the container.. it may be loaded on the host?)

  • the container may not have the right privileges to allow creating/mounting loop devices?

I am in the process of recreating the docker environment as much as I can locally so I can get away from using the gitlab CI to debug this, since the CI is quite slow for debugging these issues.

All other tests are running/passing, so that's a good sign :)

Note: this PR does NOT contain my latest changes, they're in my gitlab.com/craftyguy/pmbootstrap repo (but even those are not 100% working as indicated earlier in this comment).

@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 12, 2018

Ok I've managed to get a bit closer, by creating the /dev/loopX devices with mknod (luckily docker is run with --privileged in gitlab.com's public CI runner!).

Now I'm hung up on this:

(006052) [06:03:58] ERROR: Unable to find the partition prefix, expected the first partition of /dev/loop0 to be located at /dev/loop01 or /dev/loop0p1!

Interestingly enough, the partitions are there:


Device       Boot Start     End Sectors  Size Id Type
/dev/loop0p1 *     2048   63487   61440   30M 83 Linux
/dev/loop0p2      63488 2506751 2443264  1.2G 83 Linux

But the nodes aren't under /dev:

root@5db22b223842:/home/pmos# ls /dev/loop0*
/dev/loop0

Naturally I run kpartx to update the kernel, but that doesn't seem to be working as expected:

root@5db22b223842:/home/pmos# kpartx -l /dev/loop0
loop0p1 : 0 61440 /dev/loop0 2048
loop0p2 : 0 2443264 /dev/loop0 63488
root@5db22b223842:/home/pmos# kpartx -a /dev/loop0
root@5db22b223842:/home/pmos# ls /dev/loop0*
/dev/loop0

Even forcing it, with verbose set, shows they should have been created, but they aren't:

root@5db22b223842:/home/pmos# kpartx -a /dev/loop0 -f -v
add map loop0p1 (254:1): 0 61440 linear 7:0 2048
add map loop0p2 (254:2): 0 2443264 linear 7:0 63488

This is likely some weird docker/container magic causing this, I'll research more tomorrow. I think I'm very close to getting this to work, assuming the image creation is one of the last steps in the test_qemu_running_processes.py test!

@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 12, 2018

I've been able to get around the loop issues (apparently partitions are showing up in /dev/mapper/loopXpY in the docker container), and have patched partitions.py in my latest implementation.

There now seems to be some issue with not being able to ssh to the qemu instance that is created by the test. Not sure if it's a problem with my local docker config, but I fired it off to the gitlab.com CI just to see if it is also experiencing this issue. Getting closer!

@ollieparanoid ollieparanoid referenced this pull request Jun 14, 2018

Merged

Moving to gitlab post #65

0 of 2 tasks complete
@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 17, 2018

I do not think we can use the gitlab.com shared runners for the qemu tests, since it requires KVM and it seems there is some variability in runners that are in the 'shared runner' pool such that not all of them have KVM, enabled properly.

I am currently investigating whether I can run a runner to meet our requirements, that we should then be able to use from gitlab.com's CI.

@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 24, 2018

I've successfully created a custom gitlab runner for running pmbootstrap CI stuff (using virtualbox executor), and will host this system until we can come up with a more permanent solution. With help fom @ollieparanoid, we were able to resolve the loop device issues once and for all (hopefully).

So now all qemu-related tests are passing (xfce4 and plasma). I've enabled all pytest tests now and have about 5 failures that I will continue to debug.

@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 25, 2018

I've gotten all tests to pass, and the latest changes are force-pushed to this PR.

Here's the output/results: https://gitlab.com/craftyguy/pmbootstrap/pipelines/24474062

It's important to note that the qemu tests use a specific runner that I am hosting. I'll need access to the gitlab pmbootstrap project to configure it, or I can provide instructions on how to do so if necessary.

Total run time on the gitlab CI stuff is about 8.5 minutes, compared to ~20 minutes for Travis :P

@MartijnBraam

Code looks good 👍

- "[[ -f /home/pmos/.local/var/pmbootstrap/log.txt ]] && mv /home/pmos/.local/var/pmbootstrap/log.txt $CI_PROJECT_DIR/log.txt"
- "[[ -f /home/pmos/.local/var/pmbootstrap/log_testsuite.txt ]] && mv /home/pmos/.local/var/pmbootstrap/log_testsuite.txt $CI_PROJECT_DIR/log_testsuite.txt"
- "[[ -f /home/pmos/.config/pmbootstrap.cfg ]] && cp /home/pmos/.config/pmbootstrap.cfg $CI_PROJECT_DIR/pmbootstrap.cfg"
- "sudo dmesg > ~/$CI_PROJECT_DIR/dmesg.txt"

This comment has been minimized.

@MartijnBraam

MartijnBraam Jun 25, 2018

Member

If you set builds_dir to an absolute path in the config.toml file for your gitlab runner then $CI_PROJECT_DIR is an absolute path again like on the shared runners which makes it more consistent and easier to add more runners to the pool.

This comment has been minimized.

@craftyguy

craftyguy Jun 25, 2018

Member

Ah right, thanks for reminding me. I'll implement this, but it'll require some minor changes to my runner and some testing. It'll be a few hours before I can get to that.

@ollieparanoid

Added a few nitpicks. You did an amazing job with this, can't wait until we move to gitlab \o/ The artifacts and the increased speed are nice improvements!

@@ -0,0 +1,17 @@
#!/bin/bash

This comment has been minimized.

@ollieparanoid

ollieparanoid Jun 25, 2018

Member

How about using #!/bin/sh here? It's a matter of taste, but since Alpine and pmOS use busybox shell by default, it would be nice if this was not bash specific in my opinion.

That may also change the behavior of yes | pmbootstrap init.

This comment has been minimized.

@MartijnBraam

MartijnBraam Jun 25, 2018

Member

The scripts in the gitlab-ci.yml are executed by bash, not by sh (unless bash isn't found in the image or the runner has specifically been configured to use sh instead of bash)

This comment has been minimized.

@ollieparanoid

ollieparanoid Jun 25, 2018

Member

I think bash would use its sh compatibility mode if we changed the shebang here.

Quotes from this article:

When Bash starts in SH compatiblity mode, it tries to mimic the startup behaviour of historical versions of sh as closely as possible, while conforming to the POSIX® standard as well.
[...]
Bash starts in sh compatiblity mode when:

  • the base filename in argv[0] is sh (NB: /bin/sh may be linked to /bin/bash, but that doesn't mean it acts like /bin/bash)

This comment has been minimized.

@craftyguy

craftyguy Jun 25, 2018

Member

If bash is available on the systems these scripts are meant to be run on (gitlab runners, hopefully implied by them living in the .gitlab dir), and bash is much more powerful than sh, why wouldn't we use bash in this case?

One other thing to note, I haven't done any work to make sure these scripts run in busybox with its nuanced tool functionality differences vs the rest of the world.. because the target systems for these scripts are not using busybox versions of stuff (they are running on debian runners)..

This comment has been minimized.

@ollieparanoid

ollieparanoid Jun 26, 2018

Member

My train of thought was, that if we only have POSIX shell scripts in the repository, it would be easier to maintain because maintainers wouldn't need to know the bash specific features. But if you guys prefer bash for these scripts, let's go with that, it doesn't make much of a difference 👍

Also I was not able to reproduce the yes "" | pmbootstrap init problem with a #!/bin/bash script, so my theory of that being the cause is wrong.

# any specific runners. Specific runners are expected to provide
# all of these configurations to save time, at least for now.
[[ -d "/home/pmos" ]] && exit

This comment has been minimized.

@ollieparanoid

ollieparanoid Jun 25, 2018

Member

How about printing a message here?

# is used.
# fail quickly if run as root, since other commands here will fail
[[ "$(id -u)" == "0" ]] && exit 1

This comment has been minimized.

@ollieparanoid

ollieparanoid Jun 25, 2018

Member

How about specifying #!/bin/... -e, so it automatically fails if any of these lines fail?
Then we could get rid of all the || exit 1.

@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 26, 2018

Latest feedback incorporated, though I still have some questions about some of it (see above comments to in-line comments)

Results are here: https://gitlab.com/craftyguy/pmbootstrap/pipelines/24555986

@postmarketOS postmarketOS deleted a comment from craftyguy Jun 26, 2018

@ollieparanoid

ollieparanoid suggested changes Jun 26, 2018 edited

Thanks for the update, I think it's pretty close now!

Some more comments/discussion points:

  • I've removed my "(note to myself: does this work?)" comment, it's outdated and I didn't mean to make it part of the review (also removed your follow up question to the comment)
  • Was .gitlab/config.toml added on purpose? If not, please add it to the gitignore. I'm not sure if you have put <REDACTED> there, or if GitHub did it. In the latter case I would recommend to generate new passwords.
  • How about we run shellcheck on the gitlab CI scripts as well (by adding them to test/static_code_analysis.sh?)
  • With the modifications in test/check_checksums.py, the Travis CI code won't work as expected anymore. I think this is good, because then we don't carry around legacy code. But I would recommend that we remove the other Travis CI code as well (.travis.yml and the .travis folder).
@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 26, 2018

@ollieparanoid

Was .gitlab/config.toml added on purpose? If not, please add it to the gitignore. I'm not sure if you have put there, or if GitHub did it. In the latter case I would recommend to generate new passwords.

I added that on purpose, to serve as a reference configuration file for folks who want to also run qemu runners. I redacted the passwords, tokens myself. This file is manually updated to reflect my configuration (which is in /etc/gitlab-runner/config.toml on the server and not in any repo).
If you don't like the idea of having it here, or think it might belong somewhere else, I can remove (or move) it.

How about we run shellcheck on the gitlab CI scripts as well (by adding them to test/static_code_analysis.sh?)

I'll add this in!

With the modifications in test/check_checksums.py, the Travis CI code won't work as expected anymore. I think this is good, because then we don't carry around legacy code. But I would recommend that we remove the other Travis CI code as well (.travis.yml and the .travis folder).

Ok, I'll add the removal of the travis-related files to this PR.

@ollieparanoid

This comment has been minimized.

Member

ollieparanoid commented Jun 26, 2018

I added that on purpose, to serve as a reference configuration file for folks who want to also run qemu runners.

Thanks for explaining, that makes sense. Good idea to include that as example 👍

Clayton Craft and others added some commits Jun 5, 2018

Implement support for gitlab CI
This implements support for the gitlab CI system. Wiki, static analysis,
and build tests are implemented.
Remove Travis CI files from project
This removes Travis CI configuration, which will be replaced
by Gitlab CI
@craftyguy

This comment has been minimized.

Member

craftyguy commented Jun 27, 2018

Changes implemented, and passing gitlab CI. I suspect this will now fail Travis CI though since there's a commit in this PR to remove the travis stuff :)

https://gitlab.com/craftyguy/pmbootstrap/pipelines/24643470

@ollieparanoid

This comment has been minimized.

Member

ollieparanoid commented Jun 27, 2018

(not merging before move to gitlab, because it would disable Travis CI tests)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.