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

hebasto commented at 3:17 pm on December 28, 2019: member

This help message

0$ ./configure --help | grep -A 1 'with-gui'
1  --with-gui[=no|qt5|auto]
2                          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:

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

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

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

fanquake added the label Build system on Dec 28, 2019

fanquake added the label GUI on Dec 28, 2019

hebasto commented at 5:45 pm on December 30, 2019: member

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.

hebasto renamed this:
~~build: Misleading help for --with-gui configure option~~
build: --with-gui configure option without argument should warn if Qt not found
on Dec 30, 2019

dongcarl commented at 8:57 pm on March 11, 2020: member

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:

No --with{,out}-foo[=arg] specified ⟶ Perform detection, proceed as normal if no foo package detected
--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?

hebasto commented at 9:10 pm on March 11, 2020: member

@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:

0$ ./configure --help | grep -A 1 'with-gui'
1  --with-gui[=no|qt5|auto]
2                          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?

dongcarl commented at 9:29 pm on March 11, 2020: member

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.

hebasto commented at 7:09 pm on March 12, 2020: member

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

ryanofsky commented at 8:52 pm on March 12, 2020: member

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

Argument	Behavior
`--with-gui=auto`	Build with GUI if qt5 found, build without gui otherwise
`--with-gui=yes`	Build with GUI if qt5 found, error otherwise
`--with-gui=no`	Build without GUI
`--with-gui=qt5`	same as `--with-gui=yes`
`--with-gui`	same as `--with-gui=yes`
(none)	same as `--with-gui=auto`

Clearer help message might be:

0  --with-gui[=yes|no|auto|qt5]
1                          build bitcoin-qt GUI (default is auto,
2                          --with-gui and --with-gui=qt5 are equivalent to
3                          --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

hebasto commented at 11:21 am on March 16, 2020: member

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

ryanofsky commented at 2:03 pm on March 16, 2020: member

@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

hebasto commented at 10:23 am on March 17, 2020: member

@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?

hebasto renamed this:
~~build: --with-gui configure option without argument should warn if Qt not found~~
build: Ambiguous help string for --with-gui configure option
on Mar 17, 2020

ryanofsky commented at 11:37 am on March 17, 2020: member

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.

hebasto commented at 12:08 pm on March 17, 2020: member

@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.

ryanofsky commented at 12:54 pm on March 17, 2020: member

All good, thanks for making the other PR clearer

hebasto commented at 12:46 pm on June 13, 2020: member

Closing until Qt 6.

hebasto closed this on Jun 13, 2020

MarcoFalke locked this on Feb 15, 2022

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