Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix detection of OpenSSL built w/o deprecated features support #470

Closed
wants to merge 1 commit into from
Closed

Conversation

fxcoudert
Copy link
Contributor

@fxcoudert fxcoudert commented Sep 10, 2019

SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.

@squid-prbot
Copy link
Collaborator

Can one of the admins verify this patch?

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Sep 10, 2019
@fxcoudert
Copy link
Contributor Author

squid-anubis added the M-failed-description label

What is expected of me?

Can one of the admins verify this patch?

This pull request addresses @rousskov's comments on another (stuck) PR: #333

@rousskov
Copy link
Contributor

What is expected of me?

Please see https://github.com/measurement-factory/anubis#pull-request-labels

@rousskov
Copy link
Contributor

OK to test

@rousskov rousskov changed the title Make configure check compatible with OpenSSL 1.1 Fix detection of OpenSSL built w/o deprecated features support Sep 10, 2019
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

LGTM.

@rousskov
Copy link
Contributor

rousskov commented Sep 10, 2019

@fxcoudert, thank you for fixing this problem! Please check edited PR title and description. If you agree with them, please say so, and they will become the official commit message. Otherwise, please adjust them as needed. I will not clear this PR for commit until your confirmation.

FWIW, your discussion about an alternative implementation (that I have deleted from the PR description) was not wrong. It is just not appropriate for the commit message because, from the commit point of view, there is no reason to entertain clearly inferior solutions. I bet you detailed that more complex solution is because its direction was mentioned in PR #333, but that PR discussion was misdirected itself AFAICT. There are many obviously worse solutions. The commit message does not need to explain obvious things.

@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Sep 10, 2019
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

LGTM

@rousskov rousskov added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Sep 10, 2019
@fxcoudert
Copy link
Contributor Author

I'm OK with title and description. I didn't know you use the PR body as a commit message (I think I actually have never seen a project set up that way), so I just wanted to give a detailed justification of why I thought this was the best way to go.

@rousskov
Copy link
Contributor

I'm OK with title and description.

Great, thank you. The PR should be auto-merged soon.

I think I actually have never seen a project set up that way

Yes, the other projects that use merge bots usually force developers to cleanup development branches instead. Since our bot automatically squashes submitted changes, we both free the developers from (occasionally disrupting/unwelcomed) cleanup and have a much better control over the text of the commit message (which most developers, especially new contributors, naturally get wrong anyway). Our approach is not without cons, of course, but I think it works overall better for the Squid Project.

One of the things we currently lack is an automated message for new contributors explaining what is going on and what to expect.

squid-anubis pushed a commit that referenced this pull request Sep 12, 2019
SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Sep 12, 2019
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 13, 2019
@fxcoudert fxcoudert deleted the patch-1 branch September 13, 2019 08:13
squidadm pushed a commit to squidadm/squid that referenced this pull request Sep 17, 2019
…-cache#470)

SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.
squidadm pushed a commit to squidadm/squid that referenced this pull request Oct 14, 2019
…-cache#470)

SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.
yadij pushed a commit that referenced this pull request Oct 14, 2019
SSL_library_init() is deprecated since OpenSSL v1.1 and is absent in
OpenSSL built without deprecated features. Several distributions (e.g.
Homebrew) ship OpenSSL built without deprecated features.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants