Skip to content

Cleanup all checks for unsupported libmemcached versions #295

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

Merged
merged 1 commit into from
Jan 20, 2017
Merged

Cleanup all checks for unsupported libmemcached versions #295

merged 1 commit into from
Jan 20, 2017

Conversation

arjenschol
Copy link
Contributor

Remove all compatible checks for LIBMEMCACHED_VERSION_HEX < 0x01000002

@sodabrew sodabrew merged commit c564fd8 into php-memcached-dev:master Jan 20, 2017
@sodabrew
Copy link
Contributor

Looks great, thank you!

@sodabrew sodabrew added this to the PHP7 milestone Jan 20, 2017
@arjenschol arjenschol deleted the compatcleanup branch January 23, 2017 08:32
@malyeyev-AMZN
Copy link

Seems like one of the changes broke the build for older libmemcached (tested with 1.0.8)

awslabs/aws-elasticache-cluster-client-memcached-for-php#12

@sodabrew
Copy link
Contributor

sodabrew commented Feb 2, 2017

The removals here were too aggressive:
I missed that several of the feature tests are for 1.0.x versions, those have to be restored.

There was a bug in libmemcached 1.0.x for some 1.0.8 < x < 1.0.18 that failed to expose memcached_exists as a non-C++-mangled C symbol.
This was fixed in libmemcached here:
https://bazaar.launchpad.net/~tangent-trunk/libmemcached/1.0/revision/1037

=== modified file 'libmemcached-1.0/exist.h'
--- libmemcached-1.0/exist.h	2012-04-26 20:25:18 +0000
+++ libmemcached-1.0/exist.h	2012-06-21 05:48:37 +0000
@@ -36,6 +36,10 @@
 
 #pragma once
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 LIBMEMCACHED_API
 memcached_return_t memcached_exist(memcached_st *memc, const char *key, size_t key_length);
 
@@ -43,3 +47,6 @@
 memcached_return_t memcached_exist_by_key(memcached_st *memc,
                                           const char *group_key, size_t group_key_length,
                                           const char *key, size_t key_length);
+#ifdef __cplusplus
+}
+#endif

The effect of that bug was for library versions that had a memcached_exists but did not expose it we used the memcached_get workaround above. Removing the workaround means we're exposed to the non-exported-symbol issue now.

Since we don't compile php-memcached in C++ mode, I'm not sure if there's a way to engage the C++ name mangling so that we can gain access to the mangled memcached_exists symbol.

I'm also concerned that the unit tests did not pick this up on Travis CI. We do test against multiple versions of libmemcached, which suggests that maybe libmemcached is now in the Travis base image and we're linking to that ahead of the local copy?

@malyeyev-AMZN
Copy link

Ok, the thing is this only happens at runtime and doesn't cause build to fail

]$ make 
.
.
.
----------------------------------------------------------------------

Build complete.
Don't forget to run 'make test'.
]$ make install
Installing shared extensions:     /usr/lib64/php/7.0/modules/
]$ php -a
PHP Warning:  PHP Startup: Unable to load dynamic library '/usr/lib64/php/7.0/modules/memcached.so' - /usr/lib64/php/7.0/modules/memcached.so: undefined symbol: memcached_exist in Unknown on line 0

So this is consistent with the fact it is still building successfully even after you purged any possible libmemcached libraries in the Travis image
https://travis-ci.org/php-memcached-dev/php-memcached/builds/197770808

Also I just verified that this still happens when php-memcached is linked dynamically with libmemcached not just statically is in the original report (awslabs/aws-elasticache-cluster-client-memcached-for-php#12)

@sodabrew
Copy link
Contributor

sodabrew commented Feb 2, 2017

That makes sense, since the function is always defined in the exist.h header, just not with extern "C" { } at libmemcached's compile-time. Tests still execute on Travis CI, though, which means the module is loading at run time, so I don't understand why it doesn't reproduce there.

I looked into ways to get access to the C++ symbol, and it's basically a no-go -- the name mangling is compiler-dependent.

Options I see:

  1. Redefine the minimum supported libmemcached to be 1.0.10 (crappy because Ubuntu Trusty and Debian Wheezy ship with 1.0.8, not to mention the AWS version).
  2. Revert this commit and leave in place the old workaround for memcached_exist.
  3. Other suggestions?

REGISTER_MEMC_CLASS_CONST_LONG(OPT_REMOVE_FAILED_SERVERS, MEMCACHED_BEHAVIOR_REMOVE_FAILED_SERVERS);
#endif
#ifdef HAVE_MEMCACHED_BEHAVIOR_SERVER_TIMEOUT_LIMIT
#if defined(LIBMEMCACHED_VERSION_HEX) && LIBMEMCACHED_VERSION_HEX >= 0x01000018
REGISTER_MEMC_CLASS_CONST_LONG(OPT_SERVER_TIMEOUT_LIMIT, MEMCACHED_BEHAVIOR_SERVER_TIMEOUT_LIMIT);
Copy link
Contributor

@sodabrew sodabrew Feb 2, 2017

Choose a reason for hiding this comment

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

Is this libmemcached 1.0.18 or higher? It hasn't caused compile to fail on any older 1.0.x version.

I mis-re-read the diff.

@malyeyev-AMZN
Copy link

malyeyev-AMZN commented Feb 2, 2017

  1. ... ship with 1.0.8, not to mention the AWS version

Soon this will no longer be the case.
Could there be other reasons reason why someone would still want to intentionally use old libmemcached, say for protocol compatibility if not possible to upgrade server engine or something else?

  1. Revert this commit and leave in place the old workaround for memcached_exist.

Are there any actual downside to it besides just having old compatibility hacks clutter the codebase? Can it be partially reverted?

Tests still execute on Travis CI, though, which means the module is loading at run time, so I don't understand why it doesn't reproduce there.

Yes, I find that very odd too.

@sodabrew
Copy link
Contributor

sodabrew commented Feb 2, 2017

Ubuntu Trusty and Debian Wheezy are still in LTS until 2018, so 1.0.8 is still an important user base. Ok, finished thinking out loud, gotta revert.

sodabrew added a commit to sodabrew/php-memcached that referenced this pull request Feb 6, 2017
sodabrew added a commit to sodabrew/php-memcached that referenced this pull request Feb 6, 2017
sodabrew added a commit that referenced this pull request Feb 7, 2017
@sodabrew
Copy link
Contributor

sodabrew commented Feb 7, 2017

I've tagged php-memcached v3.0.1 which brings back the memcached_exists workaround, but keeps the other libmemcached < 1.0.2 removals. Thanks again for the cleanup work from this PR!

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

Successfully merging this pull request may close these issues.

3 participants