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

Rewrite to use Crypt::OpenSSL::Guess #104

Merged
merged 1 commit into from Apr 25, 2022

Conversation

michal-josef-spacek
Copy link
Contributor

Implementation of #97
Crypt::OpenSSL::Guess has support for all use cases implemented in this Makefile.PL

@skaji
Copy link
Contributor

skaji commented Mar 15, 2022

Makefile.PL is an auto generated file.
So you may want to edit maint/Makefile_header.PL and cpanfile.

@skaji
Copy link
Contributor

skaji commented Mar 15, 2022

LIBS => ['-lssl -lcrypto '.openssl_lib_paths()],

should be

LIBS => [openssl_lib_paths() . ' -lssl -lcrypto'],

cf akiym/Crypt-OpenSSL-Guess#12

@michal-josef-spacek
Copy link
Contributor Author

@skaji Fixed

@skaji
Copy link
Contributor

skaji commented Mar 15, 2022

CONFIGURE_REQUIRES Crypt::OpenSSL::Guess should be specified by cpanfile.

@michal-josef-spacek
Copy link
Contributor Author

@skaji ok, I updated cpanfile, test dzil build. Thanks.

@skaji
Copy link
Contributor

skaji commented Mar 15, 2022

The change looks good to me.

Note:
Before merging this PR, we should support openssl v3 because Crypt::OpenSSL::Guess may select openssl v3.

@michal-josef-spacek
Copy link
Contributor Author

@skaji Same sitation with OpenSSL 3 was before.

@michal-josef-spacek
Copy link
Contributor Author

@skaji Your #100 with -DOPENSSL_API_COMPAT=0x10000000L is working for me. I tested unit tests on OpenSSL 3.0.1 and 1.1.1l

@jonasbn jonasbn self-requested a review March 16, 2022 20:33
@mmcclimon
Copy link

+1 to this PR. I couldn't install after messing around with my openssl libs, and applying this patch got me up and running.

@jonasbn
Copy link
Collaborator

jonasbn commented Apr 8, 2022

Thanks for the feedback @mmcclimon. Easter holiday is coming up, I hope I can get it reviewed and merged and released

@jonasbn jonasbn self-assigned this Apr 19, 2022
@jonasbn jonasbn added this to the 1.9.14 milestone Apr 25, 2022
Copy link
Collaborator

@jonasbn jonasbn left a comment

Choose a reason for hiding this comment

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

LGTM, approved

@jonasbn jonasbn merged commit 20cdbdb into dsully:master Apr 25, 2022
@jonasbn
Copy link
Collaborator

jonasbn commented Apr 25, 2022

Thanks @michal-josef-spacek for the PR and thanks @skaji and @mmcclimon for review and test

@jonasbn jonasbn linked an issue Apr 25, 2022 that may be closed by this pull request
@jonasbn
Copy link
Collaborator

jonasbn commented Apr 25, 2022

I have marked this as solving #97 a TRIAL release, well let us know if this works out as expected, else we will just reopen #97

@jonasbn jonasbn linked an issue Apr 26, 2022 that may be closed by this pull request
@jonasbn
Copy link
Collaborator

jonasbn commented Apr 26, 2022

Hi @skaji, @michal-josef-spacek and @mmcclimon

This has been included in the release 1.9.14-TRIAL, just uploaded to PAUSE/CPAN.

Please let me know if you experience any issues or have any feedback. The 1.9.14 release will be made in due time depending on the outcome/feedback of the trial release.

Thanks for your contributions.

@mmcclimon
Copy link

Works for me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Idea: using Crypt::OpenSSL::Guess INC and LIBS are hardcoded
4 participants