Questions for apache gurus/code-reviewers;
- AC_CHECK_HEADERS() appears difficult to coax into accepting additional include paths, so if "--with-ssl=<path>" is specified there appears no obvious way to have AC_CHECK_HEADERS() pick up those headers in (particularly if versions exist in system locations too and we want autoconf's tests to find the <path> versions in preference to any auto-detectable ones). I've left some comments in the acinclude.m4 changes about this. For now, I've made do with adding "-I<...>" to CFLAGS prior to AC_TRY_COMPILE, but I'm sure autoconf intended some other way of handling this. For one thing, is "-I" actually portable anyway? The existing code depends utterly on it but it would be nice to do away with it altogether.
I think you mean adding -I<...> to CPPFLAGS not to CFLAGS. That should be portable and supported everywhere. It's a C preprocessor flag not a flag for the compiler. Autoconf will evaluate ac_compile which includes $CPPFLAGS. You should be able to use APR_ADDTO on CPPFLAGS for the explicit path case. (Since the user specified it, we always need to add it to CPPFLAGS.)
- My changes use autoconf tests for openssl/ssl-c headers and libraries (existing code just looks for files but doesn't actually try to use them). As a result, linker flags like -ldl, -lsocket, -lnsl, -ldld, etc are needed in advance of these tests. I've added the obvious ones I know about so that this patch can be tested as-is, but ideally Apache's builtin tests (which are obviously OK because Apache links fine) should occur before the AC_CHECK_LIB()s for "ssl" and "crypto". See "Step 3" of my changes to acinclude.m4.
A much easier route would be to include `$apr_config --libs` in that section. Those libraries would be in there. When we go to actually link, the libtool dependencies will have them there. But, you could add those to LIBS temporarily - just remove them when you are done trying to link.
- The adjustments made to LDFLAGS at the end of the testing has been written to try and match the existing stuff, but I don't confess to know what the significance of $ap_platform_runtime_link_flag is so I'm working blind there.
ap_platform_runtime_link_flag is '-R' on those platforms that need a special flag to indicate where to look at run-time for libraries. (Solaris is the predominate case.) Some platforms use '-rpath' as well. -L is not enough.
- I'm tagging "-DHAVE_OPENSSL" or "-DHAVE_SSLC" directly onto CFLAGS rather than using anything like AC_DEFINE because the latter possibility would require HAVE_OPENSSL and HAVE_SSLC to be stubbed into an appropriate "something.h.in" file. If you prefer not to have such stuff polluting CFLAGS then please suggest an appropriate ".in" file for me to hook into.
Why is having HAVE_OPENSSL or HAVE_SSLC in ap_config_auto.h a bad thing? I don't understand.
Any/all feedback most welcome.
Comments inline.
Index: acinclude.m4 =================================================================== RCS file: /home/cvspublic/httpd-2.0/acinclude.m4,v retrieving revision 1.136 diff -u -r1.136 acinclude.m4 --- acinclude.m4 17 Feb 2003 02:32:19 -0000 1.136 +++ acinclude.m4 4 Mar 2003 23:00:03 -0000 @@ -320,7 +320,7 @@ dnl and then AC_TRY_LINK to test the libraries directly for the version, dnl but that will require someone who knows how to program openssl. dnl -AC_DEFUN(APACHE_CHECK_SSL_TOOLKIT,[ +AC_DEFUN(APACHE_CHECK_SSL_TOOLKIT_OLD,[ if test "x$ap_ssltk_base" = "x"; then AC_MSG_CHECKING(for SSL/TLS toolkit base) ap_ssltk_base=""
Just toss the old APACHE_CHECK_SSL_TOOLKIT. No need to keep the old version around.
+ dnl Step 5: run version checks + if test "$ap_ssltk_type" = "openssl"; then + saved_CFLAGS=$CFLAGS + if test "x$ap_ssltk_inc" != "x"; then + CFLAGS="$CFLAGS $ap_ssltk_inc" + fi + AC_MSG_CHECKING(for OpenSSL version) + AC_TRY_COMPILE([#include <openssl/opensslv.h>], +[#if !defined(OPENSSL_VERSION_NUMBER) || OPENSSL_VERSION_NUMBER < 0x0090609f +#error "invalid openssl version" +#endif], + [dnl Replace this with OPENSSL_VERSION_TEXT from opensslv.h? + AC_MSG_RESULT(OK)],
Yes, it should indicate the version found somehow. I believe we did that before. I think it's worthwhile to have.
Index: modules/ssl/config.m4if
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/ssl/config.m4,v
retrieving revision 1.11
diff -u -r1.11 config.m4
--- modules/ssl/config.m4 29 Mar 2002 07:36:01 -0000 1.11
+++ modules/ssl/config.m4 4 Mar 2003 23:00:05 -0000
@@ -77,8 +77,13 @@
dnl # hook module into the Autoconf mechanism (--enable-ssl option)
APACHE_MODULE(ssl, [SSL/TLS support (mod_ssl)], $ssl_objs, , no, [
APACHE_CHECK_SSL_TOOLKIT
- AC_CHECK_FUNCS(SSL_set_state)
- AC_CHECK_FUNCS(SSL_set_cert_store)
+ dnl These checks aren't really useful and could fail for silly reasons
+ dnl ever the flags configured by APACHE_CHECK_SSL_TOOLKIT aren't in + dnl effect when these checks run (but are in effect during apache + dnl compilation). The version checks on openssl already make sure the + dnl below functions exist anyway. + dnl AC_CHECK_FUNCS(SSL_set_state) + dnl AC_CHECK_FUNCS(SSL_set_cert_store) ])
Just remove them altogether.
The rest of it looks good. -- justin
