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 WARNING: OpenSSL headers and library versions do not match #958

Closed
wants to merge 1 commit into from

Conversation

craig65535
Copy link
Contributor

@craig65535 craig65535 commented Aug 12, 2016

I noticed that when I specified an OpenSSL dir with --with-ssl, the configure script would still find the OpenSSL library version in the system headers, not in my specified library:

checking for OpenSSL headers version... 1.0.1 - 0x1000105fL
checking for OpenSSL library version... 1.0.2
checking for OpenSSL headers and library versions matching... no
configure: WARNING: OpenSSL headers and library versions do not match.

I looked through config.log and found that the headers check, which is done with the preprocessor (gcc -E), was not using the include directories in CPPFLAGS:

configure:22624: checking for OpenSSL headers version
configure:22654: gcc -E  conftest.c
configure:22654: $? = 0
configure:22715: result: 1.0.1 - 0x1000105fL

Here is a fix. I've changed the code that defines CPPPFLAGS. Previously, CPPPFLAGS would contain the contents of CPPFLAGS and potentially an added -P. Now, there is a variable CPPPFLAG that can either be empty or -P. This is so -P can be appended to the current value of CPPFLAGS, not whatever CPPFLAGS was when CURL_CPP_P was run.

My new output:

checking for OpenSSL headers version... 1.0.2 - 0x1000208fL
checking for OpenSSL library version... 1.0.2
checking for OpenSSL headers and library versions matching... yes

EDIT: Made an attempt at a proper fix.

@megamoose
Copy link
Contributor

Seems it is intentional ignoring --with-ssl. I found this comment in the Homebrew formula for cURL:

cURL has a new firm desire to find ssl with PKG_CONFIG_PATH instead of using

"--with-ssl" any more. "when possible, set the PKG_CONFIG_PATH environment

variable instead of using this option". Multi-SSL choice breaks w/o using it.

If that is true, --with-ssl should likely be removed instead of patched.

@bagder
Copy link
Member

bagder commented Aug 13, 2016

I disagree strongly. And comments in a homebrew tracker don't speak for the curl project.

@DomT4
Copy link

DomT4 commented Aug 13, 2016

It's worth noting Homebrew builds things in a special environment & comments in Homebrew formulae may not reflect reality outside of that environment.

@craig65535
Copy link
Contributor Author

--with-ssl works fine in terms of the build, and is not being ignored. The only thing that is broken is the OpenSSL header version check in the configure script.

@@ -33,7 +33,7 @@ AC_DEFUN([CURL_CHECK_DEF], [
AC_REQUIRE([CURL_CPP_P])dnl
OLDCPPFLAGS=$CPPFLAGS
# CPPPFLAGS comes from CURL_CPP_P
CPPFLAGS="$CPPPFLAGS"
#CPPFLAGS="$CPPPFLAGS"
Copy link
Member

Choose a reason for hiding this comment

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

So this effectively disables the CURL_CPP_P check that we added in ecf9534. If you want to remove the assignment, then remove it. If you want to remove it, then why figure out CPPPFLAGS to begin with? If you can make the script work without the CPPPFLAGS logic properly, please elaborate on how and why that works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like I said in my description, this PR is to illustrate the problem and is not a proper fix.

If you want me to come up with a better fix, I will try. If I can't, I hope someone else picks this up and the issue is not ignored. I want to be clear that the openssl header version check is now broken and the problem is easy to reproduce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made an attempt at a proper fix.

@bagder bagder added the build label Aug 14, 2016
@bagder
Copy link
Member

bagder commented Sep 4, 2016

Can you please explain the reasoning behind this patch? I don't understand why it does what it does and the comments seems to be incorrect. Ie the CURL_CPP_P macro clearly sets the variable named CPPPFLAGS (with three Ps and one S).

@craig65535
Copy link
Contributor Author

Sure, I can explain. Originally, CPPPFLAGS (the variable defined by CURL_CPP_P) was supposed to be CPPFLAGS with an optional -P added. So if your CPPFLAGS was -I /usr/local/include, CPPPFLAGS might be -I /usr/local/include -P if -P is required by your cpp.

However, I noticed that CPPPFLAGS was empty in my case while CPPFLAGS was not. This was causing the openssl check to fail. I believe this is because CURL_CPP_P was run early enough that CPPFLAGS was not defined yet.

I modified CURL_CPP_P so that it sets a new variable, CPPPFLAG, which can either be blank or just -P. So now CPPPFLAG is a string you add to CPPFLAGS, whereas originally CPPPFLAGS was a string you would replace CPPFLAGS with.

I hope I haven't added or removed a P in here and made this explanation confusing :). Let me know if you need further explanation.

@craig65535
Copy link
Contributor Author

(CURL_CPP_P is run early because it's not invoked directly, but by AC_REQUIRE)

@bagder
Copy link
Member

bagder commented Sep 4, 2016

Ah now I get it. Can you then please provide the commits squashed instead of updating an initial commit in a later one within the same pull request?

@craig65535
Copy link
Contributor Author

@bagder I squashed everything into one commit. I also eliminated the code that changes the warning into an error; let me know if I should reintroduce it.

@bagder bagder removed the needs-info label Sep 5, 2016
@bagder bagder closed this in 4639894 Sep 5, 2016
@bagder
Copy link
Member

bagder commented Sep 5, 2016

Awesome. Thanks a lot for your patch and your patience. Merged now with a slightly modified commit message!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants