Skip to content

Comments

Fix builds with new HTTP client configured with no-ocsp and no-cmp#11058

Closed
DDvO wants to merge 4 commits intoopenssl:masterfrom
mpeylo:fix_no-ocsp_no-cmp
Closed

Fix builds with new HTTP client configured with no-ocsp and no-cmp#11058
DDvO wants to merge 4 commits intoopenssl:masterfrom
mpeylo:fix_no-ocsp_no-cmp

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Feb 11, 2020

This is a fixup of #10667 for build issues newly detected by Travis due to #9982.

  • fix build for new HTTP client in case OPENSSL_NO_CMP
  • fix build with new HTTP client in case OPENSSL_NO_OCSP
  • on this occasion, fix formatting nits w.r.t. #if indentations in ocsp.h

@DDvO DDvO requested a review from mattcaswell February 11, 2020 15:17
@DDvO
Copy link
Contributor Author

DDvO commented Feb 11, 2020

Travis is happy again 😄

@DDvO
Copy link
Contributor Author

DDvO commented Feb 11, 2020

BTW, should we add OPENSSL_NO_HTTP?

mattcaswell
mattcaswell reviewed Feb 11, 2020
View reviewed changes
@mattcaswell
Copy link
Member

mattcaswell commented Feb 11, 2020

BTW, should we add OPENSSL_NO_HTTP?

I'd say no. We have far too many "no" options at the moment IMO. I don't see a need for another one.

@DDvO DDvO force-pushed the fix_no-ocsp_no-cmp branch from 5ec6c77 to 50ae0f3 Compare February 12, 2020 06:20
@DDvO
Copy link
Contributor Author

DDvO commented Feb 12, 2020

Travis now fails due to an unrelated error: unknown type name 'ECX_KEY'

@DDvO
Copy link
Contributor Author

DDvO commented Feb 12, 2020

@mattcaswell, hope you can approve the fixes in this PR regardless of the unrelated issue with ECX_KEY?
Then I'd push this as quickly as possible and however fixes the latter issue can take advantage of it.

mattcaswell
mattcaswell approved these changes Feb 12, 2020
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels Feb 12, 2020
@mattcaswell
Copy link
Member

mattcaswell commented Feb 12, 2020

I believe this is "urgent" because it is causing all PRs to show red crosses for Travis. @DDvO do you agree? I've applied the "urgent" label.

@DDvO
Copy link
Contributor Author

DDvO commented Feb 12, 2020

I believe this is "urgent" because it is causing all PRs to show red crosses for Travis.
@DDvO do you agree? I've applied the "urgent" label.

Yes, thanks, I agree.
Does the 'ugent' label mean that I can do the push right away?

@mattcaswell
Copy link
Member

mattcaswell commented Feb 12, 2020

Does the 'ugent' label mean that I can do the push right away?

In theory yes. I'd like to wait a short while to see if anyone objects, and maybe push in 2 or 3 hours if no one does.

mspncp
mspncp previously requested changes Feb 12, 2020
View reviewed changes
Copy link
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

From 3a91af68915561c052d995445ee74a012dd0d6c4 Mon Sep 17 00:00:00 2001
From: "Dr. David von Oheimb" <[email protected]>
Date: Tue, 11 Feb 2020 16:09:12 +0100
Subject: [PATCH] fix build with new HTTP client in case OPENSSL_NO_OCSP fix
 formatting nits w.r.t. #if indentations in ocsp.h

Please insert an empty line between commit title and body.

@mspncp mspncp added approval: otc review pending and removed approval: done This pull request has the required number of approvals labels Feb 12, 2020
DDvO added 3 commits February 12, 2020 10:52
@DDvO
fix build with new HTTP client in case OPENSSL_NO_OCSP
a071256 
fix also formatting nits w.r.t. #if indentations in ocsp.h
@DDvO
fixup! fix build with new HTTP client in case OPENSSL_NO_OCSP fix for…
f73b111 
…matting nits w.r.t. #if indentations in ocsp.h
@DDvO
fixup! fix build with new HTTP client in case OPENSSL_NO_OCSP fix for…
5ba557e 
…matting nits w.r.t. #if indentations in ocsp.h
@DDvO DDvO force-pushed the fix_no-ocsp_no-cmp branch from 50ae0f3 to 5ba557e Compare February 12, 2020 09:53
@DDvO
Copy link
Contributor Author

DDvO commented Feb 12, 2020 •
edited
Loading

Please insert an empty line between commit title and body.

@mspncp, as you mentioned to me elsewhere, GitHub unfortunately does not always keep newlines in commit messages - thanks for that hint!

In this case the first line was not really meant as a title but just as the first of two items.
Neverthless I've added an empty line after it
and adapted the following line such that it reads as subordinate to the first one.

@mspncp mspncp dismissed their stale review February 12, 2020 10:06

Thanks for fixing it. You may proceed to merge, since it has been approved already and is marked as 'urgent'.

@mspncp mspncp added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: otc review pending labels Feb 12, 2020
openssl-machine pushed a commit that referenced this pull request Feb 12, 2020
@DDvO
fix build for new HTTP client in case OPENSSL_NO_CMP or OPENSSL_NO_OCSP
100cc8b 
fix also formatting nits w.r.t. #if indentations in ocsp.h

Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: David von Oheimb <[email protected]>
(Merged from #11058)
@DDvO
Copy link
Contributor Author

DDvO commented Feb 12, 2020

Pushed. Thanks @mattcaswell and @mspncp!

@DDvO
Copy link
Contributor Author

DDvO commented Feb 12, 2020

I wonder why for this PR GitHub apparently did not realize that it already has been merged.
Maybe because I squashed the commits during merge?
Of course, I can also close the PR manually.

@DDvO DDvO mentioned this pull request Feb 12, 2020
Merged
@mattcaswell
Copy link
Member

mattcaswell commented Feb 12, 2020

I wonder why for this PR GitHub apparently did not realize that it already has been merged.

Because the commit hashes in this PR do not match those that eventually got pushed to master (because of rebases and addrev changes).

@DDvO DDvO deleted the fix_no-ocsp_no-cmp branch February 12, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattcaswell mattcaswell mattcaswell approved these changes

+1 more reviewer

@mspncp mspncp mspncp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge severity: urgent Fixes an urgent issue (exempt from 24h grace period)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DDvO @mattcaswell @mspncp