Opened 2 months ago

Last modified 12 days ago

#31372 merge_ready enhancement

Appveyor and Travis should use "make -k"

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci, 029-backport, 035-backport, 040-backport, 041-backport, 042-should dgoulet-merge
Cc: nickm, rl1987 Actual Points: .1
Parent ID: Points:
Reviewer: teor Sponsor:

Description (last modified by teor)

Right now our .appveyor.yml and .travis.yml are not using the "-k" make flag. That means that once it encounters an error, it stops building. In general, we would prefer our CI to keep trying to build for a long as it can, so that we can see all of the warnings and errors that are produced. This will help us save round trips between the developer and the CI process.

Child Tickets

Change History (20)

comment:1 Changed 2 months ago by teor

Travis isn't using "make -k" either, so maybe we should fix them both at the same time.

comment:2 Changed 2 months ago by nickm

Sounds like a good idea to me.

comment:3 Changed 2 months ago by teor

Description: modified (diff)
Summary: Appveyor should use "make -k"Appveyor and Travis should use "make -k"

comment:4 Changed 2 months ago by rl1987

Status: newneeds_review

comment:5 Changed 2 months ago by teor

Cc: nickm added
Keywords: 029-backport 035-backport 040-backport 041-backport added
Reviewer: teor
Status: needs_reviewneeds_revision
Type: defectenhancement

Looks good to me.

I think we should backport this change to 0.2.9 and later for consistency.
Did you want to do the backport, rl1987?
If not, I'm happy to do it when I review.

You might find the new "git-merge-forward.sh -t (test-branch-prefix)" in #31314 useful.
(But this change will cause a few conflicts, unfortunately.)

comment:6 Changed 6 weeks ago by rl1987

Feel free to do the backporting.

comment:7 Changed 6 weeks ago by rl1987

Cc: rl1987 added

comment:8 Changed 5 weeks ago by nickm

Keywords: 042-should added

Mark some needs_revision tickets as 042-should

comment:9 Changed 3 weeks ago by nickm

Owner: set to nickm
Status: needs_revisionaccepted

Changing owner.

comment:10 Changed 3 weeks ago by nickm

Status: acceptedneeds_revision

comment:11 Changed 3 weeks ago by nickm

I've made initial branches as ticket31372_029 for travis and ticket31372_appveyor_035 for appveyor.
For Travis, I just used the MAKEFLAGS variable, so that merging forward will be simpler.

For my own reference, the current PRs are:
https://github.com/torproject/tor/pull/1350 for appveyor
https://github.com/torproject/tor/pull/1351 for travis
If CI passes, I'll make a combined branch for 0.3.5 and forward, and merge-forward branches for 0.4.0, 0.4.1, and 0.4.2.

comment:12 Changed 3 weeks ago by nickm

CI passed.

ticket31372_029 (travis only) with PR: https://github.com/torproject/tor/pull/1351
ticket31372_035 (both platforms) with PR: https://github.com/torproject/tor/pull/1360
ticket31372_040 (small merge conflict) with PR: https://github.com/torproject/tor/pull/1361
ticket31372_041 (clean merge) with PR: https://github.com/torproject/tor/pull/1362
ticket31372_042 (clean merge) with PR: https://github.com/torproject/tor/pull/1363

I have also made a branch with a deliberate syntax error in mainloop.c so that we can make sure that make -k is working as in tended. That one has a PR at https://github.com/torproject/tor/pull/1364 -- do not merge it.

comment:13 Changed 3 weeks ago by nickm

Actual Points: .1
Status: needs_revisionneeds_review

CI for the real branches has passed. For the intended-to-fail branch, I have confirmed that the build continues after the first failure, and generates more output (which is what we wanted make -k to do).

comment:14 Changed 3 weeks ago by teor

I like this idea, but is there a way to summarise the errors at the end of the CI output?
Otherwise, we have to search through thousands of lines of output for the error.

comment:15 Changed 3 weeks ago by nickm

I do not know a way to summarize these errors offhand, without significant new coding.

We could run "make -k" twice, I guess? That way it will re-build whatever failed, thus outputting the errors again. That's a bit ugly though.

Usually I grep for "error:" or search for it in my browser.

comment:16 Changed 3 weeks ago by teor

I like make -k || make -k and I'm happy with the ugliness, because almost no-one will see it.
(And we can explain it in a comment, and perhaps with echo "Re-running make to show errors".)

But I am also happy to change my process, and do a search.

What do you think would be easier for us, and for new contributors?
I'll let you decide?

It's probably also worth mentioning that this change will slow down failures. I think that's ok, but I sent an email to the network team list with other ways to speed up CI.

comment:17 Changed 3 weeks ago by nickm

I think that the search approach might be better: we have to do that anyway for jenkins builders, and it's a good idea to get in practice. Do you think that's plausible? If so, let's merge_ready this.

It's probably also worth mentioning that this change will slow down failures. I think that's ok, but I sent an email to the network team list with other ways to speed up CI.

Thanks! I also think it's okay.

The tradeoff is that we find out later when there is compilation failure, but we learn everything about the compilation failure and we don't have to do multiple back-and-forth steps. Learning about a success is just as slow as it was before.

I know that Appveyor will report failure as soon as one builder has failed. Does travis do the same?

comment:18 Changed 3 weeks ago by teor

Status: needs_reviewmerge_ready

Appveyor reports failure, and doesn't run any more jobs.

Travis runs all the jobs, then reports failures. It has a fast finish mode, which is buggy, and doesn't really do what we want anyway:
https://docs.travis-ci.com/user/customizing-the-build#fast-finishing

comment:19 Changed 2 weeks ago by nickm

Keywords: dgoulet-merge added

comment:20 Changed 12 days ago by dgoulet

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

PR 1363 was merged!

Moving milestone to 041 for backport.

Note: See TracTickets for help on using tickets.