Configure: Reword the summary output#7499
Closed
mspncp wants to merge 3 commits intoopenssl:masterfrom
Closed
Conversation
In commit 820e414 (pr openssl#5247) the summary output of the Configure command was optimized towards instructing people how to create issue reports. It turned out that the wording of this message can confuse new OpenSSL users and make them think that they are seeing an error message. This commit makes the summary output start with a success to prevent a misunderstanding. Also it gives more hints to new OpenSSL users.
mspncp
added
branch: master
levitte
requested changes
Oct 25, 2018
Member
levitte
left a comment
levitte
left a comment
There was a problem hiding this comment.
Nits to pick, looks good otherwise!
Configure
Outdated
| *** OpenSSL has been successfully configured *** | ||
| *** *** | ||
| *** perl configdata.pm --dump *** | ||
| *** If you encounter a problem during building, please open an *** |
Configure
Outdated
| *** perl configdata.pm --dump *** | ||
| *** *** | ||
| *** (If you are new to OpenSSL, you might want to consult the *** | ||
| *** 'Troubleshooting' section in the INSTALL file first.) *** |
Member
There was a problem hiding this comment.
I'm not entirely sure, but I think the period before the closing parenthesis can be removed.
Contributor
Author
There was a problem hiding this comment.
On second thought: it's a complete sentence, so I would miss the period. Thoughts @openssl/nativespeakers?
Contributor
Author
|
Richard, is it correct that this box appears only in the case of success? Otherwise, the heading line would have to be conditional. |
Member
|
Yes, correct |
levitte
approved these changes
Oct 25, 2018
Contributor
Author
|
Thanks, Richard. I'll sleep a night over it before merging to give others the opportunity for feedback. |
mspncp
added
the
approval: done
levitte
pushed a commit
that referenced
this pull request
Oct 26, 2018
The failure of RAND_load_file was only noticed because of the heap corruption which was reported in #7499 and fixed in commit 5b4cb38. To prevent this in the future, RAND_load_file() now explicitly checks RAND_status() and reports an error if it fails. Related-to: #7449 Reviewed-by: Paul Dale <[email protected]> (Merged from #7456)
mspncp
added a commit
to mspncp/openssl
that referenced
this pull request
Oct 26, 2018
The failure of RAND_load_file was only noticed because of the heap corruption which was reported in openssl#7499 and fixed in commit 5b4cb38. To prevent this in the future, RAND_load_file() now explicitly checks RAND_status() and reports an error if it fails. Related-to: openssl#7449 Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#7456)
mattcaswell
approved these changes
Oct 26, 2018
levitte
pushed a commit
that referenced
this pull request
Oct 26, 2018
In commit 820e414 (pr #5247) the summary output of the Configure command was optimized towards instructing people how to create issue reports. It turned out that the wording of this message can confuse new OpenSSL users and make them think that they are seeing an error message. This commit makes the summary output start with a success to prevent a misunderstanding. Also it gives more hints to new OpenSSL users. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #7499) (cherry picked from commit 41349b5)
levitte
pushed a commit
that referenced
this pull request
Oct 26, 2018
In commit 820e414 (pr #5247) the summary output of the Configure command was optimized towards instructing people how to create issue reports. It turned out that the wording of this message can confuse new OpenSSL users and make them think that they are seeing an error message. This commit makes the summary output start with a success to prevent a misunderstanding. Also it gives more hints to new OpenSSL users. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #7499)
Contributor
Author
|
Merged to master and 1.1.1. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Problem
In commit 820e414 (pr #5247) the summary output of the Configure command was optimized towards instructing people how to create issue reports.
It turned out that the wording of this message can confuse new OpenSSL users and make them think that they are seeing an error message.
No, I'm not joking. If you don't believe it, have a look at #7068 and #7212, it happened already twice. @mattcaswell is my witness, he was involved in both. You can read the threads starting at comments
Proposal
This commit makes the summary output start with a friendly success message to prevent a misunderstanding from the very beginning. Also it gives more hints to new OpenSSL users.