add_c_file: bug when adding to end of include.am

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

changed milestone to %Tor: 0.4.3.x-final

added 043-should actualpoints::.4 component::core tor/tor milestone::Tor: 0.4.3.x-final owner::nickm points::.1 priority::medium resolution::fixed reviewer::catalyst severity::normal status::closed type::defect labels

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.)

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

Trac:
Actualpoints: N/A to .1
Status: assigned to needs_review

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()?

Trac:
Status: needs_review to merge_ready

Trac:
Reviewer: N/A to catalyst

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?

• master: https://github.com/torproject/tor/pull/1667

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

Trac:
Status: merge_ready to needs_review
Actualpoints: .1 to .3

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?

• master: https://github.com/torproject/tor/pull/1667

I had to rebase to avoid conflicts with the include.am split from #32137 (moved). 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.

Trac:
Status: needs_review to merge_ready

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.

Trac:
Status: merge_ready to needs_revision

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.)

Trac:
Actualpoints: .3 to .4
Status: needs_revision to merge_ready

Merged to master.

Merged #32962 (moved), #32984 (moved), and #20218 (moved) together.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 48m

added 3h 12m of time spent

mentioned in issue #32984 (moved)

moved to tpo/core/tor#32962 (closed)