Opened 4 months ago

Closed 6 weeks ago

#32137 closed task (implemented)

Split {feature,core,app}/*/include.am out of core/include.am

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-design
Cc: nickm Actual Points: .2
Parent ID: #31851 Points: 0.5
Reviewer: catalyst Sponsor: Sponsor31-can

Description

It seems a bit weird that we're missing feature/include.am, maybe it's time to fix that.

Child Tickets

Change History (8)

comment:1 Changed 4 months ago by nickm

The original rationale was that since we do not yet have good (downward dependencies only) factoring for the layers higher than src/lib, we can't yet build them as separate libraries. But you're right that this shouldn't stop us from having separate include files here.

If we take this approach, we should probably ahve one include.am file per subdirectory, as we do for src/lib, and just base them on a different template.

comment:2 Changed 6 weeks ago by nickm

Summary: Split feature/include.am out of core/include.amSplit {feature,core,app}/*/include.am out of core/include.am

Update title to reflect desired layout.

comment:3 Changed 6 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:4 Changed 6 weeks ago by nickm

Actual Points: .2
Status: acceptedneeds_review

See branch ticket32137 with PR at https://github.com/torproject/tor/pull/1657 . I anticipate some conflicts with #32487, but they should be tractable.

comment:5 Changed 6 weeks ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

comment:6 Changed 6 weeks ago by dgoulet

Reviewer: catalyst

comment:7 in reply to:  4 Changed 6 weeks ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

See branch ticket32137 with PR at https://github.com/torproject/tor/pull/1657 . I anticipate some conflicts with #32487, but they should be tractable.

Other than the existing bugs noted in #32962, this looks good.

comment:8 Changed 6 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Thank you for the review, catalyst! Merging to master.

Note: See TracTickets for help on using tickets.