Opened 4 months ago

Closed 3 months ago

#22636 closed defect (implemented)

Add Travis configs so GitHub forks get CI coverage

Reported by: catalyst Owned by: patrickod
Priority: High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: continuous-integration ci testing best-practice unit-testing new-developers travis review-group-21
Cc: isis, patrickod Actual Points: 1
Parent ID: Points: .5
Reviewer: nickm Sponsor:

Description

It would be useful to have Travis configs for tor so GitHub users can have CI coverage for changes in their forks. This should probably run make check or whatever more strict level of tests we converge on later.

Child Tickets

Change History (26)

comment:1 Changed 4 months ago by isis

Cc: patrickod added
Keywords: ci added

CCing Patrick from NoiseTor on this, since (IIRC) it was originally his idea and I believe he is still interested in working on this.

comment:2 Changed 4 months ago by dgoulet

Milestone: Tor: unspecified

comment:3 Changed 4 months ago by isis

One of the things we've been talking about in the Rust channel is that it would be great if the .travis.yml had a target in it's build matrix for building with rust support. (See https://trac.torproject.org/projects/tor/wiki/RustInTor for how to build.) I'm not sure the best Travis way to do this, as it may involve adding a pre-build section which does rustup, or just say that rust is the target language and expect it to also have a decent gcc installed.

comment:4 Changed 4 months ago by patrickod

Yep still interested in tackling this. I'll hopefully have time in the coming week to get this done.

comment:5 Changed 3 months ago by nickm

Keywords: continuous-integration testing best-practice unit-testing new-developers travis added
Priority: MediumHigh

comment:6 Changed 3 months ago by isis

Owner: set to patrickod
Status: newassigned

comment:7 Changed 3 months ago by isis

Actual Points: .5
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Points: .5
Status: assignedneeds_review

Hi! Patrick (patrickod) did a bunch of work on this, and I did a bit of work on top of that. Our changes are in my bug22636 branch, and as you can see, CI is passing. :)

Note that it passing depends on the minor fix for #22830 (changing $HOME to $CARGO_HOME in src/rust/tor_util/include.am, which I've included in this branch in commit 4c580a609e.

If you have any questions about the whats or the whys of any of the directives in .travis.yml, please feel free to ask.

comment:8 Changed 3 months ago by isis

Oh actually, this needs a changes file. Let me do that.

comment:9 in reply to:  8 Changed 3 months ago by isis

Replying to isis:

Oh actually, this needs a changes file. Let me do that.


Okay done.

comment:10 Changed 3 months ago by isis

Also this is totally safe to back-port to… well everything. (Not only safe, but potentially an improvement for people trying to make bugfixes on older releases.) Should I make branches to port it back to everything listed in https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases#Current ?

comment:11 Changed 3 months ago by catalyst

Reviewer: catalyst

comment:12 Changed 3 months ago by nickm

Sounds fine -- actually, if you just do a backport to 0.2.4, then when I merge to maint-0.2.4, it should merge forward cleanly to everything else.

comment:13 in reply to:  12 Changed 3 months ago by isis

Replying to nickm:

Sounds fine -- actually, if you just do a backport to 0.2.4, then when I merge to maint-0.2.4, it should merge forward cleanly to everything else.


Okay, done in my bug22636_0.2.4 branch. This is for 0.2.4 through 0.3.0. I rebased out all the rust toolchain stuff, since that would fail and also waste time. Tests pass.

My bug22636_0.3.1 branch is for 0.3.1. Tests pass.

comment:14 Changed 3 months ago by catalyst

Status: needs_reviewneeds_revision

Thanks! This is very useful to have. Technically this looks mostly good to me, with a few minor (mostly documentation) nits.

I have a GitHub account with Travis CI enabled. I confirm that tests on bug22636_0.3.1 pass and tests on bug22636_0.2.4 pass.

I'm trying to understand the differences between this and our Jenkins configurations. It looks like our Jenkins config turns on --disable-silent-rules to get a bit more verbosity on the compile lines; should we do likewise? Jenkins also does make check instead of make check-spaces and make test. Is there some reason to not do make check? I think that doesn't significantly increase the run time, but I haven't measured the difference yet.

Commenting the .travis.yml with brief explanations of these choices might be a good idea. Also, squashing the commits somewhat would be good. Some of the commit messages, like the ones involving Rust config choices, might work better as comments in .travis.yml.

comment:15 in reply to:  14 ; Changed 3 months ago by isis

Replying to catalyst:

Thanks! This is very useful to have. Technically this looks mostly good to me, with a few minor (mostly documentation) nits.

I have a GitHub account with Travis CI enabled. I confirm that tests on bug22636_0.3.1 pass and tests on bug22636_0.2.4 pass.

I'm trying to understand the differences between this and our Jenkins configurations. It looks like our Jenkins config turns on --disable-silent-rules to get a bit more verbosity on the compile lines; should we do likewise?


Yes, this is a good idea.

Jenkins also does make check instead of make check-spaces and make test. Is there some reason to not do make check? I think that doesn't significantly increase the run time, but I haven't measured the difference yet.


This is probably a good idea, as it tests more. The output might be a bit tricky to get, because the way it is configured right now, if some step fails, the whole CI build will fail fast. So e.g. if compilation failed, don't waste more time testing. Also if make check-spaces fails, your commit needs to be fixed up anyway, so again don't bother wasting time testing. Whereas if we run make check it does all this in one go, and if it fails some step, it only reports that at the end of everything. Also, all the output that we'd need in order to see what failed gets shoved into a file (but possibly I can get that output with a after-script stanza?).

Commenting the .travis.yml with brief explanations of these choices might be a good idea. Also, squashing the commits somewhat would be good. Some of the commit messages, like the ones involving Rust config choices, might work better as comments in .travis.yml.


Yes, this is a great idea.

comment:16 in reply to:  15 Changed 3 months ago by isis

Replying to isis:

Replying to catalyst:

Thanks! This is very useful to have. Technically this looks mostly good to me, with a few minor (mostly documentation) nits.

I have a GitHub account with Travis CI enabled. I confirm that tests on bug22636_0.3.1 pass and tests on bug22636_0.2.4 pass.

I'm trying to understand the differences between this and our Jenkins configurations. It looks like our Jenkins config turns on --disable-silent-rules to get a bit more verbosity on the compile lines; should we do likewise?


Yes, this is a good idea.


Okay, this is done in commit 63928a0b1294324249c69c8165e13c808e11a335.

Jenkins also does make check instead of make check-spaces and make test. Is there some reason to not do make check? I think that doesn't significantly increase the run time, but I haven't measured the difference yet.


This is probably a good idea, as it tests more. The output might be a bit tricky to get, because the way it is configured right now, if some step fails, the whole CI build will fail fast. So e.g. if compilation failed, don't waste more time testing. Also if make check-spaces fails, your commit needs to be fixed up anyway, so again don't bother wasting time testing. Whereas if we run make check it does all this in one go, and if it fails some step, it only reports that at the end of everything. Also, all the output that we'd need in order to see what failed gets shoved into a file (but possibly I can get that output with a after-script stanza?).


This is also done in commit 63928a0b1294324249c69c8165e13c808e11a335.

(Note that the build is failing right now, but that's only because it doesn't include the fix for #22830 yet.)

Commenting the .travis.yml with brief explanations of these choices might be a good idea. Also, squashing the commits somewhat would be good. Some of the commit messages, like the ones involving Rust config choices, might work better as comments in .travis.yml.


Yes, this is a great idea.


Done in commit ca4168ea98818b1f291449596a7885671aefeded.

I'll squash all the branches once these new changes are approved.

Last edited 3 months ago by isis (previous) (diff)

comment:17 Changed 3 months ago by isis

Status: needs_revisionneeds_review

comment:18 Changed 3 months ago by isis

Keywords: review-group-21 added

Selfishly putting tickets I care about in review-group-21. :)

comment:19 Changed 3 months ago by isis

Note that the squashed branches are now bug22636_0.2.4_squashed here and bug22636_0.3.1_squashed here. CI for each one pass (because the 0.3.1 branch is now based on a version of maint-0.3.1 that includes the patch for #22830).

comment:20 Changed 3 months ago by nickm

Reviewer: catalystnickm

comment:21 Changed 3 months ago by nickm

Hi! I have some questions:

  • If we merge this, will it start spamming the #tor-bots IRC?
  • Do we want to consider adding --enable-fragile-hardening?
  • Should we install the optional dependencies (zstd, lzma2, scrypt, ...)
  • In the homebrew stanza, should all of those lines have "brew outdated openssl" or only the openssl one?
  • Should we skip rust installation if we won't be building with rust, in order to save time?

comment:22 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

Putting this in needs_revision, but please feel free to put it back into needs_review if there are no problems above.

comment:23 Changed 3 months ago by nickm

Oh, one more thing:

  • --enable-fatal-warnings was called --enable-gcc-warnings before 0.2.9.x

comment:24 Changed 3 months ago by isis

Actual Points: .51
Status: needs_revisionneeds_review

Replying to nickm:

Hi! I have some questions:

  • If we merge this, will it start spamming the #tor-bots IRC?


It will spam #tor-bots when someone makes a commit which breaks things when the commit's parent was previously working, and it will also spam for a commit which fixes things when the commit's parent was previously broken. Do we want different behaviour?

  • Do we want to consider adding --enable-fragile-hardening?


We can do that, if that is what we expect to pass. (I think that's a reasonable expectation. Also, side note, we should make sure to document our expectations in doc/HACKING better!) Added in commit c91a57ccf9.

  • Should we install the optional dependencies (zstd, lzma2, scrypt, ...)


Sure, done in commit 1bb00fb812. (Except, for some reason, Ubuntu Trusty doesn't have a libzstd package available.)

  • In the homebrew stanza, should all of those lines have "brew outdated openssl" or only the openssl one?


Oh oops, good catch. Fixed in commit 8f8689f70235.

  • Should we skip rust installation if we won't be building with rust, in order to save time?


Yes, this is a great idea. Done in commit e5dd07a4c64. It doesn't seem to save any time though, probably if I had to guess because there's some transparent proxy setup with a cache.

I've updated bug22636_0.2.4_squashed and bug22636_0.3.1_squashed and tests are passing respectively.

Replying to nickm:

Oh, one more thing:

--enable-fatal-warnings was called --enable-gcc-warnings before 0.2.9.x


Oh right. Thanks! There's now a bug22636_0.2.9 branch for 0.2.9 and 0.3.0 which contains everything from the bug22636_0.2.4_squashed branch, except it uses --enable-gcc-warnings instead.

Version 0, edited 3 months ago by isis (next)

comment:25 in reply to:  24 Changed 3 months ago by nickm

Replying to isis:

Replying to nickm:

Hi! I have some questions:

  • If we merge this, will it start spamming the #tor-bots IRC?


It will spam #tor-bots when someone makes a commit which breaks things when the commit's parent was previously working, and it will also spam for a commit which fixes things when the commit's parent was previously broken. Do we want different behaviour?

Maybe we only want that for some people? I don't think we care if some person we've never heard of starts breaking their github tor branches.

  • Do we want to consider adding --enable-fragile-hardening?


We can do that, if that is what we expect to pass. (I think that's a reasonable expectation. Also, side note, we should make sure to document our expectations in doc/HACKING better!) Added in commit c91a57ccf9.

Cool

  • Should we install the optional dependencies (zstd, lzma2, scrypt, ...)


Sure, done in commit 1bb00fb812. (Except, for some reason, Ubuntu Trusty doesn't have a libzstd package available.)

Well, at least we tried :)

  • In the homebrew stanza, should all of those lines have "brew outdated openssl" or only the openssl one?


Oh oops, good catch. Fixed in commit 8f8689f70235.

  • Should we skip rust installation if we won't be building with rust, in order to save time?


Yes, this is a great idea. Done in commit e5dd07a4c64. It doesn't seem to save any time though, probably if I had to guess because there's some transparent proxy setup with a cache.

I've updated bug22636_0.2.4_squashed and bug22636_0.3.1_squashed and tests are passing respectively.

Replying to nickm:

Oh, one more thing:

--enable-fatal-warnings was called --enable-gcc-warnings before 0.2.9.x


Oh right. Thanks! There's now a bug22636_0.2.9 branch for 0.2.9 and 0.3.0 which contains everything from the bug22636_0.2.4_squashed branch, except it uses --enable-fatal-warnings instead. bug22636_0.2.4_squashed has now been changed to use --enable-gcc-warnings.

Sounds good! I'll give it all another look over, and merge.

comment:26 Changed 3 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed
m
 e
  r
   g
    e
     d

!

Now let's see what happens.

Note: See TracTickets for help on using tickets.