Opened 3 years ago

Closed 3 years ago

#23564 closed task (implemented)

run make check-changes in CI

Reported by: catalyst Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ci tor-doc tor-releng
Cc: Actual Points:
Parent ID: #23562 Points:
Reviewer: Sponsor:


We should run make check-changes in our CI automation (Travis, oniongit, etc.) so we can make sure our changes files are release-ready at or before merge time.

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by catalyst

We might also want to make it do something reasonable when the changes directory is empty:

Traceback (most recent call last):
  File "./scripts/maint/", line 86, in <module>
  File "./scripts/maint/", line 41, in lintfile
    with open(fname) as f:
IOError: [Errno 2] No such file or directory: './changes/*'
Makefile:11633: recipe for target 'check-changes' failed
make: *** [check-changes] Error 1

comment:2 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 years ago by nickm

Status: acceptedneeds_review

Let's see if my branch ticket23564 helps here? It tries to handle the above by making "check-changes" part of make check, and to fix the most common false positive cases.

comment:4 Changed 3 years ago by catalyst

Status: needs_reviewneeds_revision

It looks like a good start! Seems to produce false negatives if $(top_srcdir) is, say, .. The dotfile check probably should be against os.path.basename(f) and should probably be in the __main__ clause. The file walk seems like it might only go one level deep; this is probably OK if we document it, otherwise consider using something like os.walk()?

Also, typo ("Ticked" vs "Ticket"):

-        warn("bugfix does not say 'Fixes bug XXX'")
+        warn("Ticked marked as bugfix, but does not say 'Fixes bug XXX'")

comment:5 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

Thanks for the review, catalyst!

The dotfile thing was wrong, but for another reason: It should have applied inside the listdir loop, on the outputs of listdir.

I've gone with the non-walk approach, since the changes directory does not contain directories itself.

Fixup commit pushed to ticket23564. Better now?

comment:6 Changed 3 years ago by catalyst

Status: needs_reviewmerge_ready

Looks good; thanks! changes/bug23610 on master needs fixing up when merging because it currently fails check-changes.

comment:7 Changed 3 years ago by nickm

Squashed and merged; "make check" still passes. :)

comment:8 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.