autoconf: add 'test' alias for 'tests' to configure #14492

kallewoof commented at 3:51 AM on October 16, 2018: member

I often get --disable-test and --disable-tests mixed up. This PR makes both do the same thing and in addition allows --without-test[s] as well (i.e. it adds 3 aliases to the use_tests variable in configure.ac).

The help output:

./configure --help
[...]
  --enable-upnp-default   if UPNP is enabled, turn it on at startup (default
                          is no)
  --disable-test,
  --disable-tests         do not compile tests (default is to compile)
  --disable-gui-tests     do not compile GUI tests (default is to compile if
                          GUI and tests enabled)
[...]
  --with-miniupnpc        enable UPNP (default is yes if libminiupnpc is
                          found)
  --without-test,
  --without-tests         alias for --disable-tests
  --with-rapidcheck       enable RapidCheck property based tests (default is
                          yes if librapidcheck is found)
[...]

These commands:

./configure
./configure --enable-test
./configure --enable-tests
./configure --with-test
./configure --with-tests

all result in

  with test     = yes

while these:

./configure --disable-test
./configure --disable-tests
./configure --without-test
./configure --without-tests

give

  with test     = no

Combining these (e.g. --enable-tests --without-test) does not break anything, but some of them take precedence over the others, so the outcome depends on which one you are specifying. There's no reason to ever mix them, so this should not be an issue.

fanquake added the label Build system on Oct 16, 2018

promag commented at 9:27 AM on October 16, 2018: member

From https://ftp.gnu.org/old-gnu/Manuals/autoconf-2.53/html_node/Pretty-Help-Strings.html:

Properly formatting the help strings which are used in AC_ARG_WITH (see External Software) and AC_ARG_ENABLE (see Package Options) can be challenging.

So NACK AC_ARG_WITH aliases.

The other alias I'm -0, don't think build scripts should offer ambiguity.

kallewoof commented at 9:39 AM on October 16, 2018: member

So NACK AC_ARG_WITH aliases.

The 'with' versus 'enable' make sense from a package perspective (you compile WITH libzmq, and you ENABLE internal tests), but this is simply a way of wording things. Note that we at the end say "with tests" despite using the enable alternative. I'm fairly convinced this is a special case worth considering, despite GNU docs.

As a sidenote, we are currently using 'with(out)-gui' despite it being an internal feature. It may be a part of why I keep mixing these up, actually.

I'm fine with dropping the without part, but I am hopeful I can not have to run configure twice all the time as I keep writing 'disable-test' to match up e.g. 'disable-bench'.

The other alias I'm -0, don't think build scripts should offer ambiguity.

There is nothing ambiguous about accepting two versions for the same configuration option.

promag commented at 12:07 AM on October 18, 2018: member

As a sidenote, we are currently using 'with(out)-gui' despite it being an internal feature. It may be a part of why I keep mixing these up, actually.

From a quick look these should use AC_ARG_ENABLE

--without-daemon                     --with-daemon                     -- build bitcoind daemon (default=yes)
--without-gui                        --with-gui                        -- build bitcoin-qt GUI (default=auto)
--without-libs                       --with-libs                       -- build libraries (default=yes)
--without-utils                      --with-utils                      -- build bitcoin-cli bitcoin-tx (default=yes)

IMO the distinction between --enable and --with is nice and we could avoid mixing that.

What I think could help is to error if an unknown option is set which currently is ignored. cc @theuni (?)

kallewoof commented at 1:13 AM on October 18, 2018: member

@promag Yeah that makes sense to me. People have been using these for years though, so I'm not sure it's gonna be easy to convince people.

fanquake requested review from theuni on Oct 18, 2018

autoconf: add 'test' alias for 'tests' to configure f8fde05d59

autoconf: --without-test[s] aliases 2f350bdc87

kallewoof force-pushed on Oct 18, 2018

laanwj commented at 8:07 AM on October 18, 2018: member

not sure about this; yes, it's user friendlier to "guess the right thing" on typos, and I don't think there's any possible ambiguity here

on the other hand, I don't personally know enough about obscure m4 dialects to be confident about this not breaking something on some platform/autoconf/...

What I think could help is to error if an unknown option is set which currently is ignored. cc @theuni (?)

I've been frustrated myself at times at invalid options silently being accepted and ignored, but I think this is not possible, or at least greatly complicated due to the pass-through logic. For example—it is possible to pass secp256k1 configure arguments with regard to assembly to the top-level configure script.

@promag Yeah that makes sense to me. People have been using these for years though, so I'm not sure it's gonna be easy to convince people.

Yes, that would be more correct. Not worth the bother IMO.

kallewoof commented at 8:16 AM on October 18, 2018: member

@laanwj I'm fairly sure autoconf works in exactly the same way in this regard (process chronological order and execute no-value case in order of appearance), no matter the platform/architecture/dialect. If not, it would probably break other things left and right in subtle and horrifying ways.

(Am I really the only person who ever gets these two wrong? I literally get it wrong 50% of the time, even today.)

sipa commented at 8:35 AM on October 18, 2018: member

@kallewoof I just use tab completion after ./configure , and I don't think I ever noticed the inconsistency between the cmdline option and what is printed in the summary.

kallewoof closed this on Nov 30, 2018

kallewoof deleted the branch on Nov 30, 2018

DrahtBot locked this on Sep 8, 2021