Opened 5 weeks ago

Closed 5 weeks ago

Last modified 5 weeks ago

#32124 closed defect (fixed)

Interpret --disable-module-dirauth=no correctly

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.1-alpha
Severity: Normal Keywords: 042-backport, network-team-roadmap-october, tor-build BugSmashFund
Cc: nickm, dgoulet Actual Points: 0.2
Parent ID: #32123 Points: 0.1
Reviewer: nickm Sponsor: Sponsor31-can

Description (last modified by teor)

Currently, we treat --disable-module-dirauth=no as enabling the C macro, but disabling the Makefile variable.

Apparently lots of people make this mistake:

The most common mistake for this macro is to consider the two actions as action-if-enabled and action-if-disabled.

This is not the case!

Since using --disable-foo or --enable-foo=no are equivalent, for the macro, you cannot really use this macro with those meanings. 

https://autotools.io/autoconf/arguments.html

I don't know if we should backport this change, it just didn't work before, so maybe it should just go in master?

Child Tickets

Change History (10)

comment:1 Changed 5 weeks ago by teor

Description: modified (diff)

comment:3 Changed 5 weeks ago by teor

Status: assignedneeds_review

comment:4 Changed 5 weeks ago by teor

Still needs a changes file, I'll split it out once we decide how far to backport.

comment:5 Changed 5 weeks ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

I'd suggest 042 and forward, but master-only is also okay. The fix looks fine.

Do we make this mistake anywhere else?

Please feel free to merge once there's a changes file.

comment:6 in reply to:  5 Changed 5 weeks ago by teor

Replying to nickm:

I'd suggest 042 and forward, but master-only is also okay. The fix looks fine.

Do we make this mistake anywhere else?

We made a similar mistake for tcmalloc, all the other uses of AC_ARG_{WITH,WITHOUT,ENABLE,DISABLE} are fine.

Please feel free to merge once there's a changes file.

I'll do a similar fix, and merge both to 0.4.2 and later.

comment:7 Changed 5 weeks ago by teor

Actual Points: 0.2
Status: needs_revisionneeds_review

We also made a similar mistake with jemalloc, although that one looks like a typo.

So I'm putting this back up for review, feel free to merge it straight after review.

See my pull request:

The master merge is clean, the test branch is at:

comment:8 Changed 5 weeks ago by teor

Keywords: 042-backport added; no-backport? removed

comment:9 Changed 5 weeks ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final
Resolution: fixed
Status: needs_reviewclosed

LGTM; merged to 0.4.2 and forward.

comment:10 Changed 5 weeks ago by nickm

Keywords: BugSmashFund added
Note: See TracTickets for help on using tickets.