build: Ambiguous help string for --with-gui configure option #17813

This help message

$ ./configure --help | grep -A 1 'with-gui'
  --with-gui[=no|qt5|auto]
                          build bitcoin-qt GUI (default=auto)

states that --with-gui and --with-gui=auto are equivalent.

But this is not correct.

Consider a system without available Qt:

$ ./configure --with-gui | grep 'with gui'
configure: error: Qt dependencies not found

$ ./configure --with-gui=auto | grep 'with gui'
configure: WARNING: Qt dependencies not found; bitcoin-qt frontend will not be built
  with gui / qt = no

What should be fixed: the help message or configure behavior?

The related @laanwj's comment from #6938:

The default if you don't provide it is auto, which means that (with this pull), it will

• build with Qt5 if Qt5 is installed
• otherwise, build with Qt4 if Qt4 is installed
• otherwise, build without GUI

It seems --with-gui without an argument should not cause a configure error.

To me, this doesn't follow how I expect configure scripts to work... For a package foo that has auto as the default, I would expect that:

1. No --with{,out}-foo[=arg] specified ⟶ Perform detection, proceed as normal if no foo package detected
2. --with-foo specified ⟶ equivalent to --with-foo=yes, fail is no foo package detected

In our case, we should have --with-gui act exactly the same as --with-gui=yes (and perhaps --with-gui=qt5 if I'm understanding correctly) which I think is the current behaviour?

@dongcarl

In our case, we should have --with-gui act exactly the same as --with-gui=yes (and perhaps --with-gui=qt5 if I'm understanding correctly) which I think is the current behaviour?

As noted in the OP, the configure help message states the opposite:

$ ./configure --help | grep -A 1 'with-gui'
  --with-gui[=no|qt5|auto]
                          build bitcoin-qt GUI (default=auto)

and not default=yes or default=qt5. And we have no yes value for this option.

Maybe the help message is incorrect?

My understanding is that configure flag defaults are describing the default when no flags are specified, not when we have --with-gui with no argument.

IRC #bitcoin-builds log:

<hebasto> dongcarl: IIUC, you say that '--with-gui' acts exactly the same as '--with-gui=yes'. But this option has no 'yes' value. How it should work? <dongcarl> hebasto: I think we should make '--with-gui' = <dongcarl> sorry mistyped <dongcarl> '--with-gui' == '--with-gui=yes' == '--with-gui=qt5' <hebasto> When both qt4 and qt5 were available such assumption was wrong. When we will have more than one option to build GUI again, we'll receive a trouble again to think what 'yes' does mean. <hebasto> that's why '--with-gui' == '--with-gui=auto' is preferable, imho <dongcarl> hebasto: If we have more than one option to build GUI again, think '--with-gui=yes' should mean '--with-gui=auto' BUT with the exception that if nothing is detected, we will error

@hebasto asked me to comment, and what Carl suggested seems right:

Clearer help message might be:

  --with-gui[=yes|no|auto|qt5]
                          build bitcoin-qt GUI (default is auto,
                          --with-gui and --with-gui=qt5 are equivalent to
                          --with-gui=yes)

This probably would make more sense as an --enable option instead of a --with option, but that doesn't seem worth changing

@ryanofsky Thank you for such comprehensive description. Implemented in #18361.

@ryanofsky Thank you for such comprehensive description. Implemented in #18361.

I'm confused because I thought this issue was reporting an ambiguous help string and that my comment was just providing a better help string and description of current behavior.

It looks like what might be happening is #18297 might be changing the --with-gui to do something different than what it is doing now, and that #18361 which is built on #18297 might be changing it back to do the same thing that it is doing now, and also updating the help string.

I'd advise separating these things and just making a PR to just clarify the help string if you think it needs clarification, and keeping #18297 separate. Otherwise, I don't think it's even very important to update the help string because as carl says #17813 (comment) it's possible to interpret it correctly even now

Very much recommend PRs that do one thing at a time, keep refactoring separate from behavior changes, and explicitly state what behavior is changing when it is changing

@ryanofsky

I'm confused because I thought this issue was reporting an ambiguous help string and that my comment was just providing a better help string and description of current behavior.

Sorry for confusing. The question in this issue was:

What should be fixed: the help message or configure behavior?

Basing on @laanwj's comment I initially have suggested the changes to configure behavior in #17836 (which is closed now).

Then, thanks to your and Carl's clarifications, it became clear that only the help string is ambiguous and requires a bit of modification. That was suggested in #18361.

I'd advise separating these things and just making a PR to just clarify the help string if you think it needs clarification, and keeping #18297 separate. Otherwise, I don't think it's even very important to update the help string because as carl says #17813 (comment) it's possible to interpret it correctly even now

The rationale: as #18297 introduces a regression (on Windows configure fails if depends are built with DEBUG=1), #18298 seems its natural followup. To make changes in #18298 clear and easy to review refactoring of AC_ARG_WITH([gui]...) is required, what is done in #18361.

Keeping in mind the goal is #18307, the suggested chain of changes is: #18297 -> #18361 -> #18298 -> #18307.

IIUC, separating #18361 from other prs will make things easier to reviewers on the path to #18307, right?

IIUC, separating #18361 from other prs will make things easier to reviewers on the path to #18307, right?

I don't know enough #18361 to be able to evaluate it. The PR description is not good, and I'd suggest you remove the first line that says it "is an implementation of comprehensive description of --with-gui option provided by ryanofsky" because I think that's misleading. It makes it sound like the changes in #18361 were suggested by me, which they weren't. All I was trying to do in my comment was suggest a help string that might describe current behavior better. I don't even think there is a problem with current help string. It's just a little less complete and clear than it could be, and I only commented because you asked me to look at this in IRC.

I think if you want to make progress on #18361 and other PRs it would help if you give PRs descriptions that state how new behavior differs from old behavior, and don't require opening links to other issues and discussions to understand.

@ryanofsky

I don't know enough #18361 to be able to evaluate it. The PR description is not good, and I'd suggest you remove the first line that says it "is an implementation of comprehensive description of --with-gui option provided by ryanofsky" because I think that's misleading. It makes it sound like the changes in #18361 were suggested by me, which they weren't. All I was trying to do in my comment was suggest a help string that might describe current behavior better. I don't even think there is a problem with current help string. It's just a little less complete and clear than it could be, and I only commented because you asked me to look at this in IRC.

The #18361 description has been updated. Sorry for misleading.

I think if you want to make progress on #18361 and other PRs it would help if you give PRs descriptions that state how new behavior differs from old behavior, and don't require opening links to other issues and discussions to understand.

That is my bad habit to facilitate pr describing by links. Will try to improve myself.

All good, thanks for making the other PR clearer

Closing until Qt 6.