fix: restore missing --help in Configure, clean up output and show enabled/disabled flags, warn user of possible incorrect usage #22621
Conversation
|
Looks like you need a rebasee, but otherwise, LGTM |
BMDan
force-pushed
the
fix/restore-missing-Configure-help
branch
from
2ac50a3 to
e1f6a61
Compare
Rebased, updated PR summary with more details, and fixed a bug that got in the way of displaying the status of flags affected by cascades. |
|
In the future it would also be nice to sort the long lists of flags, platforms, etc. into alphabetically sorted columns so that they can be read easily. It's almost impossible to read them as is. But that should be done in a separate PR. |
mattcaswell
added
the
hold: cla required
|
This does not look like it qualifies for "CLA: trivial". Please can you submit a CLA for this: |
github-actions
bot
added
the
severity: fips change
|
This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html |
|
CLA has been signed; what do I need to do to refresh the CLA check? |
openssl-machine
removed
the
hold: cla required
t8m
added
the
hold: cla required
|
Could you please use |
github-actions
bot
removed
the
severity: fips change
|
This PR has the label 'hold: cla required' and is stale: it has not been updated in 30 days. Note that this PR may be automatically closed in the future if no CLA is provided. For CLA help see https://www.openssl.org/policies/cla.html |
BMDan
force-pushed
the
fix/restore-missing-Configure-help
branch
from
e1f6a61 to
2c05fa9
Compare
openssl-machine
removed
the
hold: cla required
BMDan
changed the title
|
This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section). |
|
@hlandau do you have a moment to take a look at the changes I've made, please? |
t8m
added
branch: master
t8m
added
approval: otc review pending
triaged: feature
|
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 61 days ago |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 91 days ago |
|
This PR is in a state where it requires action by @openssl/otc but the last update was 122 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago |
Co-authored-by: Viktor Dukhovni <viktor1ghub@dukhovni.org>
|
👋 @vdukhovni or @t8m ? |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
| if (not exists $disabled{$i}) | ||
| { | ||
| $this_color=$en_color; | ||
| $i = uc $i; |
There was a problem hiding this comment.
Not sure I really like the upper casing here. Traditionally these things have always been written lowercase.
There was a problem hiding this comment.
Both uppercasing and color are used to indicate which options are enabled or disabled. There are two circumstances where color alone wouldn't suffice: if color's not available and/or enabled for the terminal—which we could usually programmatically detect—or if the user is unable to distinguish between colors—which we cannot.
I can certainly leave out the uppercasing logic, but that leaves color as the only mechanism by means of which this information will be communicated to the user.
Which approach would you prefer?
|
This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago |
|
This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago |
| if (not exists $disabled{$i}) | ||
| { | ||
| $this_color=$en_color; | ||
| $i = uc $i; |
In 081436b (from #11230), the ability to use
--helpas a flag toconfigandConfigurewas lost. The flag would be worth restoring on its own merits, but it's also needed to restore the accuracy of references to it on the Wiki.This PR restores the ability to get
--help. While we're at it, we also include a few helpful related features/fixes:\nto the end of usage output to stderr.CFLAGS/CXXFLAGS. However, this can lead to confusion, since autoconf and friends use flag syntax (e.g.--with-zlib), whereas Configure uses bareword (with-zlib). Indeed, in this regard, the Wiki continues to be incorrect even after this PR is merged ("If you provide a [sic] option not known to configure or ask for help, then you get a brief help message", emphasis mine). As changed here, when the user passes an argument like--enable-zstd, if it would have been a valid Configure flag without the leading--, Configure will now issue a "NOTE" to the user on stderr.A brief postscript: please remember when reviewing this patch that I bear no responsibility for the style of indentation chosen in this file, though I did my best to follow it faithfully.
Output of new version
Output of new version with CLICOLOR enabled
Output of new version reflecting a disable_cascade argument having been applied
Output of new version related to feature/fix numbered 4, above:
Configureparameter with--:Configureparameter with--if it is unambiguously not meant for Configure to consume:Configureparameter:CLA is signed.