Opened 4 months ago

Last modified 3 weeks ago

#32230 needs_revision defect

configure summary is confusing or incorrect

Reported by: teor Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version: Tor: 0.4.3.1-alpha
Severity: Normal Keywords: tor-build, 044-should
Cc: nickm Actual Points:
Parent ID: Points: 0.5
Reviewer: teor Sponsor:

Description

Some of the options in the new configure summary are confusing or incorrect. The configure summary was introduced in #31373.

Wrong place:

  • maybe --enable-systemd should be an optional library?

Inverted, should be --enable-* :

  • --disable-seccomp
  • --disable-libscrypt
  • --disable-gcc-hardening ?
  • --disable-linker-hardening ?
  • --disable-module-dirauth
  • --disable-unittests

Remove Double-Negative ? :

  • assert()s disabled (--disable-asserts-in-tests, dev only): no
    • assert()s enabled (--enable-asserts-in-tests, dev only): yes

Broken:

  • --enable-gcc-warnings:
    • is not Verbose Warnings
    • is an alias for --enable-fatal-warnings, which is already listed
    • did you mean --disable-gcc-warnings-advisory ?
  • --disable-asciidoc
    • also disables manpages and HTML, but they are shown as enabled
  • --enable-fragile-hardening
    • should be true if --enable-expensive-hardening is set, but is shown as false

Missing:

  • --disable-module-relay
    • please build the module feature summary from the list of modules in configure
  • --disable-asciidoc
  • --enable-lzma
  • --enable-zstd
  • --enable-cargo-online-mode
  • --with-tor-user=[user]
  • --with-tor-group=[group]
  • and a few others
  • and a whole bunch of env vars

Child Tickets

Change History (9)

comment:1 in reply to:  description ; Changed 4 weeks ago by dgoulet

Reviewer: teor
Status: assignedneeds_review

Replying to teor:

Some of the options in the new configure summary are confusing or incorrect. The configure summary was introduced in #31373.

Wrong place:

  • maybe --enable-systemd should be an optional library?

Agree.

Inverted, should be --enable-* :

  • --disable-seccomp
  • --disable-libscrypt
  • --disable-gcc-hardening ?
  • --disable-linker-hardening ?
  • --disable-module-dirauth
  • --disable-unittests

All these are enabled by default and thus why we put in the --disable. If one does autocompletion on ./configure --enable-, none of the above shows up.

Remove Double-Negative ? :

  • assert()s disabled (--disable-asserts-in-tests, dev only): no
    • assert()s enabled (--enable-asserts-in-tests, dev only): yes

Agree.

Broken:

  • --enable-gcc-warnings:
    • is not Verbose Warnings
    • is an alias for --enable-fatal-warnings, which is already listed
    • did you mean --disable-gcc-warnings-advisory ?

I did not realize it was an alias and it is deprecated. I will remove it.

  • --disable-asciidoc
    • also disables manpages and HTML, but they are shown as enabled

Added!

  • --enable-fragile-hardening
    • should be true if --enable-expensive-hardening is set, but is shown as false

Fixed.

Missing:

  • --disable-module-relay

That one is there.

  • please build the module feature summary from the list of modules in configure

Done.

  • --disable-asciidoc

Fixed.

  • --enable-lzma
  • --enable-zstd
  • --enable-cargo-online-mode

Added the above.

  • --with-tor-user=[user]
  • --with-tor-group=[group]

I have no clue where this is used. I can't find TORUSER or TORGROUP being used anywhere?

  • and a few others
  • and a whole bunch of env vars

For now this is a good improvement! We can add the rest as we notice them.

Branch: ticket32230_043_01
PR: https://github.com/torproject/tor/pull/1701

(Putting teor as a reviewer in order to confirm if this is acceptable from their original post).

comment:2 in reply to:  1 ; Changed 3 weeks ago by teor

Cc: nickm added
Status: needs_reviewneeds_revision

Replying to dgoulet:

Replying to teor:

Inverted, should be --enable-* :

  • --disable-seccomp
  • --disable-libscrypt
  • --disable-gcc-hardening ?
  • --disable-linker-hardening ?
  • --disable-module-dirauth
  • --disable-unittests

All these are enabled by default and thus why we put in the --disable. If one does autocompletion on ./configure --enable-, none of the above shows up.

Ok, but the output is still wrong:

test "x$enable_seccomp" != "xno" && value=1 || value=0
PPRINT_PROP_BOOL([libseccomp (--disable-seccomp)], $value)

If enable_seccomp is yes, then value is 1, and we print --disable-seccomp yes.
But we should actually print one of these:

  • --enable-seccomp yes
  • --disable-seccomp no

I prefer the output without the double-negative.
But I can understand if you want to print the non-default option as well.

  • --with-tor-user=[user]
  • --with-tor-group=[group]

I have no clue where this is used. I can't find TORUSER or TORGROUP being used anywhere?

Maybe nickm can help?

  • and a few others
  • and a whole bunch of env vars

For now this is a good improvement! We can add the rest as we notice them.

Yes, I agree. A good summary doesn't need everything. Just the important and useful things.

Branch: ticket32230_043_01
PR: https://github.com/torproject/tor/pull/1701

I'll do a full review once the PR has been revised.

comment:3 in reply to:  2 Changed 3 weeks ago by nickm

Replying to teor:

Replying to dgoulet:

  • --with-tor-user=[user]
  • --with-tor-group=[group]

I have no clue where this is used. I can't find TORUSER or TORGROUP being used anywhere?

Maybe nickm can help?

I can't find any evidence of these fields either. They used to be used by some of the scripts in contrib/dist, but all those scripts are now removed, since they were obsolete and unmaintained.

comment:4 in reply to:  2 Changed 3 weeks ago by dgoulet

Status: needs_revisionneeds_review

Replying to teor:

Replying to dgoulet:

Replying to teor:

Inverted, should be --enable-* :

  • --disable-seccomp
  • --disable-libscrypt
  • --disable-gcc-hardening ?
  • --disable-linker-hardening ?
  • --disable-module-dirauth
  • --disable-unittests

All these are enabled by default and thus why we put in the --disable. If one does autocompletion on ./configure --enable-, none of the above shows up.

Ok, but the output is still wrong:

test "x$enable_seccomp" != "xno" && value=1 || value=0
PPRINT_PROP_BOOL([libseccomp (--disable-seccomp)], $value)

If enable_seccomp is yes, then value is 1, and we print --disable-seccomp yes.
But we should actually print one of these:

  • --enable-seccomp yes
  • --disable-seccomp no

I prefer the output without the double-negative.
But I can understand if you want to print the non-default option as well.

Right, the string is hardcoded regardless of the value of the option. Basically, the --disable-seccomp is the configure option that is available, regardless if you enabled it or not.

The way to look at it is libseccomp = yes/no. The configure options there were added so the user knows how to switch the option on or off.

I agree it is confusing. We would need to handle different text output for _each_ option depending on their values. Maybe another ticket?

Another way to look at it is that --enable-seccomp virtually doesn't exists, you either have no option which enables seccomp by default or you disable it with --disable.

comment:5 Changed 3 weeks ago by teor

Status: needs_reviewneeds_revision

Can we try a slightly different format, that includes the value of each configure option?

For example:

libseccomp (--disable-seccomp=no)    yes

This format makes the double-negative explicit, so it may be less confusing.

We could generate "yes" or "no" based on a boolean argument, rather than printing the option value directly? If that's easier?

comment:6 Changed 3 weeks ago by dgoulet

Keywords: 043-must removed
Milestone: Tor: 0.4.3.x-finalTor: unspecified

I've tried for 5 min to add that value and the m4 world is not allowing me for some reasons. I think because the moon is unaligned and too much voodoo with autoconf/m4.......

I'll drop this for now, I can't spent more time on this :S.

comment:7 Changed 3 weeks ago by teor

Keywords: tor-build 044-should added
Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Status: needs_revisionmerge_ready
Version: Tor: unspecifiedTor: 0.4.3.1-alpha

Ok, let's merge what we have in 0.4.3, and then see if we can do better in 0.4.4 ?

We need to fix a typo in the changes file, the configure summary was only released in 0.4.3.1-alpha.

Then we can merge this PR:

But keep the ticket open.

comment:8 Changed 3 weeks ago by teor

Status: merge_readyneeds_revision

Fixed and merged to master; leaving open for further fixes in 0.4.4.

comment:9 Changed 3 weeks ago by dgoulet

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final
Note: See TracTickets for help on using tickets.