Opened 12 months ago

Last modified 5 months ago

#32193 needs_review enhancement

update .gitlab-ci.yml to remove broken cruft and add a complete test suite

Reported by: eighthave Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: Android, gitlab-ci, tor-ci, 043-can
Cc: hiro, catalyst Actual Points:
Parent ID: Points:
Reviewer: hiro, catalyst Sponsor:

Description

This removes the old, broken cruft from the GitLab CI setup, and adds a whole range of new jobs. The most valuable is complete Android builds with NDK r17b and r20, then a limited test run in an Android emulator.

Since I was working on ./configure, I added tests across the GNU/Linux distros. The jobs are added in order of value, so the commits on top could easily be omitted as needed.

This currently does not limit when these jobs runs, and it probably should. Most of the jobs are various GNU/Linux distros and releases, which should be the same for most things. For all those tests, I think they should probably be run only on the stable release branches, e.g.:

...
.test-template: &test-template
  only:
    - /^release-.*$/
  script:
...

@hiro @catalyst pinging you since you did the previous edits on .gitlab-ci.yml.

The code is here:
https://github.com/torproject/tor/pull/1448

Child Tickets

TicketTypeStatusOwnerSummary
#33213defectclosedremove obsolete mirroring job from .gitlab-ci.yml

Change History (18)

comment:2 Changed 12 months ago by nickm

Milestone: Tor: 0.4.3.x-final
Status: newneeds_review

comment:3 Changed 12 months ago by nickm

Component: Core TorCore Tor/Tor

comment:4 Changed 12 months ago by dgoulet

Reviewer: dgoulet

comment:5 Changed 10 months ago by ahf

Keywords: tor-ci added

comment:6 Changed 9 months ago by nickm

Keywords: 043-can added

tag all currently needs_review tickets with 043-can. (Since there's code before the feature freeze, maybe we can take it.)

comment:7 Changed 9 months ago by dgoulet

Reviewer: dgoulethiro, catalyst

So this is quite large change to the .gitlab.yaml file but I have no way to confirm it works or makes senses actually as I have no clue what that file does.

@hiro, @catalyst: makes sense here?

comment:8 Changed 9 months ago by eighthave

If you push the existing .gitlab-ci.yml to any gitlab.com fork, you will see that the CI job results in a broken build. In my fork, you can see the jobs working. Each job has a full log, that is literally just a log of the bash script running, e.g. the script: sections of .gitlab-ci.yml

comment:9 in reply to:  8 Changed 9 months ago by catalyst

Replying to eighthave:

If you push the existing .gitlab-ci.yml to any gitlab.com fork, you will see that the CI job results in a broken build. In my fork, you can see the jobs working. Each job has a full log, that is literally just a log of the bash script running, e.g. the script: sections of .gitlab-ci.yml

Could you please show an example of the current .gitlab-ci.yml on master producing a failure? I've been running CI pipelines for branches based on master on gitlab.com for a long time (months? more than a year?) without extraneous failures.

comment:10 in reply to:  7 ; Changed 9 months ago by catalyst

Replying to dgoulet:

So this is quite large change to the .gitlab.yaml file but I have no way to confirm it works or makes senses actually as I have no clue what that file does.

@hiro, @catalyst: makes sense here?

I agree that this is a fairly large change. The individual commits are mostly well-structured, so I suggest breaking it into multiple tickets. The removal of the broken mirroring job seems fairly simple and safe, and I'll make a new ticket for it.

There's also the question of how many of these jobs should run by default, because as we've seen on GitHub, that can slow things down enough to be a barrier to contributing.

comment:11 in reply to:  10 Changed 9 months ago by catalyst

Replying to catalyst:

There's also the question of how many of these jobs should run by default, because as we've seen on GitHub, that can slow things down enough to be a barrier to contributing.

The below run time (from the example branch linked above) seems excessively slow for our default CI. I think we need to run far fewer jobs by default.

15 jobs for android-emulator-test-runs in 107 minutes and 23 seconds (queued for 3 seconds)

comment:12 Changed 9 months ago by catalyst

Cc: tlyu removed

Remove the tlyu username that I don't use on this site

comment:13 Changed 9 months ago by eighthave

gitlab-ci has very flexible job scheduling. Very few of these should run on every commit or merge request. How you want them scheduled it up to you. I'd run one of each main platform regularly on merge requests and forks, then do the full coverage only after commits have been merged into the official master. Or perhaps even only on stable release branches.

Given my experience with merge requests to tor, this one is a great example, I don't think the timing of an 1-2 hour long CI job run is going to affect much on the submitter side when takes 4 months for the merge request to be reviewed.

The update job is the old broken cruft. It doesn't do anything anymore, there is no more _oniongit.eu_.

comment:14 in reply to:  8 Changed 8 months ago by catalyst

Replying to eighthave:

If you push the existing .gitlab-ci.yml to any gitlab.com fork, you will see that the CI job results in a broken build. In my fork, you can see the jobs working. Each job has a full log, that is literally just a log of the bash script running, e.g. the script: sections of .gitlab-ci.yml

You mentioned this earlier. I don't see evidence of such build failures when I push stuff to gitlab.com. See the successful build at https://gitlab.com/argonblue/tor/-/commits/ticket33213 for example. That is literally your pull request branch rewound to only include the mirroring job removal commit and one additional commit to add a changes file.

I would like to prioritize the remaining patches based on knowing how broken the .gitlab-ci.yml is after removing the mirroring job. Would you please tell us what kinds of failures you experienced with .gitlab-ci.yml that are attributable to the version of that file that is on master?

comment:15 Changed 8 months ago by eighthave

Sorry, I don't remember any details at this point, its been a few months.

comment:16 in reply to:  15 Changed 8 months ago by catalyst

Replying to eighthave:

Sorry, I don't remember any details at this point, its been a few months.

Thanks. Based on my observations, it's currently not a problem on master.

The changes that parameterize the Debian-based builds using templates mostly look good. I'll need to look a bit more closely at them and learn more about how templates work on GitLab.

There are some choices you made that might not be appropriate to our repository. I'll try to figure out which they are and try to ask about your rationale for them. Also I'll try to determine whether they're appropriate choices for us. We might not accept your patches as-is, but it seems somewhat likely that we'll use something based on them (with credit to you, of course).

I've asked someone on the Tor Browser team to take a look at the Android work when they get a chance.

comment:17 Changed 8 months ago by eighthave

I know gitlab CI quite well at this point and really like it because it is only a thin layer on top of existing tools like YAML, Docker, bash, etc. I'm happy to answer questions so that you can get this working for Tor. Its fine by me if you use these just as an example. The served my purpose of ensuring I didn't break other platforms while I was working on the Android TorService and giving CI builds for Android.

For example, the templates are just YAML templates, they aren't really specific to GitLab-CI.

Also, F-Droid has a bunch of server capacity for GitLab-CI runners, we'd be happy to share with Tor, just let me know.

comment:18 Changed 5 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

0.4.3 was released: Move non merge-ready 0.4.3 tickets to 044.

Note: See TracTickets for help on using tickets.