Opened 7 months ago

Closed 7 months ago

#32962 closed defect (fixed)

add_c_file: bug when adding to end of include.am

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 043-should
Cc: Actual Points: .4
Parent ID: Points: .1
Reviewer: catalyst Sponsor:

Description

Taylor reports that add_c_file behaves incorrectly when adding an item to the end of a list in an automake chunk.

Child Tickets

Change History (9)

comment:1 Changed 7 months ago by catalyst

Also the ./ prefix is not being stripped from the filename prior to being inserted in the automake chunk, which also causes it to not sort correctly. (despite the usage example showing a ./ prefix.)

comment:2 Changed 7 months ago by nickm

Actual Points: .1
Status: assignedneeds_review

I've got a patch for these at ticket32962 with PR at https://github.com/torproject/tor/pull/1665

comment:3 in reply to:  2 ; Changed 7 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

I've got a patch for these at ticket32962 with PR at https://github.com/torproject/tor/pull/1665

Thanks! Looks good.

Nit: maybe we could use os.path.relpath() for consistency with topdir_file()?

comment:4 Changed 7 months ago by catalyst

Reviewer: catalyst

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

Actual Points: .1.3
Status: merge_readyneeds_review

Replying to catalyst:

Replying to nickm:

I've got a patch for these at ticket32962 with PR at https://github.com/torproject/tor/pull/1665

Thanks! Looks good.

Nit: maybe we could use os.path.relpath() for consistency with topdir_file()?

I found the path handling in this script really hard to understand. So I created a PR that distinguishes tor-relative (topdir, renamed to tordir) and tor/src-relative (srcdir) paths. I also made the canonicalisation more reliable.

What do you think?

I had to rebase to avoid conflicts with the include.am split from #32137.

comment:6 in reply to:  5 Changed 7 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to teor:

I found the path handling in this script really hard to understand. So I created a PR that distinguishes tor-relative (topdir, renamed to tordir) and tor/src-relative (srcdir) paths. I also made the canonicalisation more reliable.

What do you think?

I had to rebase to avoid conflicts with the include.am split from #32137.

Thanks! This is a lot easier to understand now. Looks good to me.

Nits:

  • Maybe we should avoid using assert() for invalid user input? I prefer to reserve assertions for internal inconsistencies. On the other hand, this is a maintainer tool, and I suspect it's not especially confusing if there's an assertion failure?
  • I ran this in a build directory, whereupon it created the files but couldn't find the include.am to modify. Maybe we should also look for a src/Makefile.am?

These are fairly minor and I'm also ok with merging this as is.

comment:7 Changed 7 months ago by teor

Status: merge_readyneeds_revision

I'm happy to change the asserts to raise ValueErrors instead, for consistency with the rest of the script.

I'm also happy to change these checks to look for the Makefile.am. Seems like an easy mistake for a maintainer to make.

I'll fix these once I'm back at my workstation.

comment:8 Changed 7 months ago by teor

Actual Points: .3.4
Status: needs_revisionmerge_ready

I've fixed the errors, and I'll merge once CI passes.
(Although we don't have CI on this script, it's still good practice.)

comment:9 Changed 7 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to master.

Merged #32962, #32984, and #20218 together.

Note: See TracTickets for help on using tickets.