Fix WARNING: OpenSSL headers and library versions do not match #958

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@craig65535
Contributor
craig65535 commented Aug 12, 2016 edited

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
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
Member
bagder commented Aug 13, 2016

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

@DomT4
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
Contributor

--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.

@bagder bagder and 1 other commented on an outdated diff Aug 14, 2016
@@ -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"
@bagder
bagder Aug 14, 2016 Member

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.

@craig65535
craig65535 Aug 15, 2016 edited Contributor

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.

@craig65535
craig65535 Aug 15, 2016 Contributor

I made an attempt at a proper fix.

@bagder bagder added the build label Aug 14, 2016
@bagder
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
Contributor

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
Contributor

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

@bagder
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 craig65535 CPPPFLAGS is now CPPPFLAG. Fixes CURL_CHECK_DEF.
140092b
@craig65535
Contributor

@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 added a commit that closed this pull request Sep 5, 2016
@craig65535 @bagder craig65535 + bagder configure: make the cpp -P detection not clobber CPPFLAGS
CPPPFLAGS is now CPPPFLAG. Fixes CURL_CHECK_DEF.

Fixes #958
4639894
@bagder bagder closed this in 4639894 Sep 5, 2016
@bagder
Member
bagder commented Sep 5, 2016

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

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