Opened 8 months ago

Closed 3 months ago

#32143 closed task (fixed)

Build some CI jobs with ALL_BUGS_ARE_FATAL

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-ci
Cc: nickm Actual Points:
Parent ID: Points: 0.2
Reviewer: teor Sponsor:

Description

Follow-up to #32141.

We should probably build with ALL_BUGS_ARE_FATAL for some of the check, stem, and chutney jobs. (But not for coverage or distcheck.)

Child Tickets

Change History (15)

comment:1 Changed 7 months ago by ahf

It doesn't sound like this is blocking the release of 0.4.2. Should we move it to 0.4.3 with the current backlog of 0.4.2 tickets still being in place?

comment:2 Changed 7 months ago by teor

Keywords: 043-should added; 042-should removed
Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final

comment:3 Changed 7 months ago by dgoulet

Parent ID: #32141

comment:4 Changed 5 months ago by ahf

Keywords: tor-ci added

comment:5 Changed 3 months ago by rl1987

Status: newneeds_review

https://github.com/torproject/tor/pull/1777

Let's allow failures for the new CI jobs for time being, as they might be fairly fragile.

Bugs #33546 and #33544 were discovered.

comment:6 Changed 3 months ago by asn

Reviewer: nickm

comment:7 Changed 3 months ago by nickm

Reviewer: nickmteor
Status: needs_reviewneeds_revision

I've added a comment about the way that we're invoking configure here, and one about CFLAGS is handled.

Additionally, I'd like to request additional review from Teor if possible, on the question of CI speed. Teor has done good work recently on CI performance, and I'd like their opinion on:

  • whether this will slow down CI much,
  • whether there is any sensible workaround for that,
  • and whether we should try to apply the workaround before or after merge.

comment:8 Changed 3 months ago by rl1987

Status: needs_revisionneeds_review

The above has been addressed. Test build: https://travis-ci.org/github/rl1987/tor/builds/662084047

The failure on one of the jobs is #33544.

comment:9 in reply to:  7 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

Replying to nickm:

I've added a comment about the way that we're invoking configure here, and one about CFLAGS is handled.

Additionally, I'd like to request additional review from Teor if possible, on the question of CI speed. Teor has done good work recently on CI performance, and I'd like their opinion on:

  • whether this will slow down CI much,
  • whether there is any sensible workaround for that,
  • and whether we should try to apply the workaround before or after merge.

Thanks Nick!

I'd like to avoid adding any extra jobs, if possible. Instead, we could:

  • try to build all existing jobs with ALL_BUGS_ARE_FATAL, unless there's some reason we want to turn hardening options off (coverage, distcheck)
  • make ALL_BUGS_ARE_FATAL apply to Appveyor, as well as Travis

If that causes too many CI failures, we could:

  • add a few jobs on Travis. I recommended a list of jobs we could add, without adding too much extra time. (Due to Travis parallelism.)
  • stop using ALL_BUGS_ARE_FATAL on Appveyor.

comment:10 Changed 3 months ago by teor

In general, I'd like to avoid using allow_failure, because it doesn't get us much value:

  • it slows down CI, and
  • it slows down developers, because we have to manually check every PR for failures, and decide if they are bad or not.

comment:11 Changed 3 months ago by teor

Oh, and if we're skipping some unit tests when ALL_BUGS_ARE_FATAL, let's try to make sure that we still run those tests on some configurations, particularly rare configurations (disabled modules, NSS, macOS).

comment:12 Changed 3 months ago by rl1987

Status: needs_revisionneeds_review

Made further changes. If this is good now, I can squash/cleanup git history into new branch and open new pull request.

comment:13 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

Almost there, but let's use #ifndef COCCI rather than excluding a whole file from coccinelle.

comment:14 Changed 3 months ago by rl1987

Status: needs_revisionneeds_review

comment:15 Changed 3 months ago by teor

Keywords: 043-should removed
Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final
Resolution: fixed
Status: needs_reviewclosed

Thanks!

Squashed, removed reverted commit and its revert, squashed some commits that used the old option name, and merged to master.

Note: See TracTickets for help on using tickets.