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 build failure and automake support #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jdolinski
Copy link

@jdolinski jdolinski commented Apr 13, 2017

#9 #8 #7

@kmaleyev I created a new pull request to more clearly define the changes and incorporate your feedback. I have tested the build on Ubuntu 14 and Amazon Linux 2017.03

I organized this PR more cleanly and discarded some changes after your clarification and feedback. I tried squashing PR #10 but it got really ugly and wanted to be clear on the changes.

Let me know your thoughts, thanks in advance.

@@ -71,7 +71,7 @@

#include <algorithm>

Copy link
Author

@jdolinski jdolinski Apr 13, 2017

Choose a reason for hiding this comment

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

The linking of the test cases and memcached files was running into the error below. I had renamed a couple methods that contained the same signature causing multiple definition errors.

CXXLD tests/libmemcached-1.0/testapp tests/libmemcached-1.0/tests_libmemcached_1_0_testapp-dynamic_mode_test.o: In functionget_world':
/usr/include/c++/4.8/sstream:129: multiple definition of get_world' tests/libmemcached-1.0/tests_libmemcached_1_0_testapp-all_tests.o:/home/ubuntu/new/aws-elasticache-cluster-client-libmemcached/BUILD/../tests/libmemcached-1.0/all_tests.cc:75: first defined here tests/libmemcached-1.0/tests_libmemcached_1_0_testapp-dynamic_mode_test.o:(.data.rel+0x0): multiple definition of collection'
tests/libmemcached-1.0/tests_libmemcached_1_0_testapp-all_tests.o:(.data.rel+0x0): first defined here
/usr/bin/ld: Warning: size of symbol collection' changed from 2272 in tests/libmemcached-1.0/tests_libmemcached_1_0_testapp-all_tests.o to 64 in tests/libmemcached-1.0/tests_libmemcached_1_0_testapp-dynamic_mode_test.o collect2: error: ld returned 1 exit status

@@ -9,6 +9,5 @@ nobase_include_HEADERS+= libmemcached-1.0/struct/memcached.h
nobase_include_HEADERS+= libmemcached-1.0/struct/result.h
nobase_include_HEADERS+= libmemcached-1.0/struct/sasl.h
nobase_include_HEADERS+= libmemcached-1.0/struct/server.h
nobase_include_HEADERS+= libmemcached-1.0/struct/configuration_server.h
Copy link
Author

Choose a reason for hiding this comment

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

Dead code

Copy link
Author

@jdolinski jdolinski left a comment

Choose a reason for hiding this comment

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

Modified the automake include files to the memcached dependencies required for the test cases during compilation/linking

@@ -24,8 +24,7 @@ noinst_HEADERS+= libmemcached/continuum.hpp
noinst_HEADERS+= libmemcached/do.hpp
noinst_HEADERS+= libmemcached/encoding_key.h
noinst_HEADERS+= libmemcached/error.hpp
noinst_HEADERS+= libmemcached/flag.hpp
noinst_HEADERS+= libmemcached/host.h
Copy link
Author

Choose a reason for hiding this comment

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

Dead code

@@ -192,6 +193,7 @@ $(TMP_DIR):
@$(mkdir_p) $(TMP_DIR)

libtest_unittest_LDADD+= libtest/libtest.la
libtest_unittest_LDADD+= $(libmemcached_libmemcached_la_OBJECTS)
libtest_unittest_SOURCES= libtest/unittest.cc
Copy link
Author

Choose a reason for hiding this comment

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

Modified the automake include files to fix compiling/linking errors when referencing memcached functions

Copy link
Author

@jdolinski jdolinski left a comment

Choose a reason for hiding this comment

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

Removed files that will be regenerated during the autoreconf process. For example "configure" & "Makefile.in"

@jdolinski
Copy link
Author

jdolinski commented Apr 13, 2017

Hi Kosty,
Sorry about submitting a second pull request but I had problems squashing. Once I received your feedback, I realized I had made some assumptions that were not true and needed to back some things out. Look forward to your feedback and thoughts.

@jdolinski jdolinski mentioned this pull request Apr 13, 2017
These are not needed
These are not needed
@raphaelquati
Copy link

I'm trying to use your PR, to create a docker image to run in ECR.
I'm using PHP-7.1.5 image (debian:jessie).

apt install automake autoconf libtool git shtool autogen
git clone ......
cd .....
rm -rf .git
autoreconf -vfi
mkdir BUILD
cd BUILD
../configure --prefix=$(pwd)/local --with-pic

The configure gives error:

checking for cc... cc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether cc accepts -g... yes
checking for cc option to accept ISO C89... none needed
configure: error: cannot find install-sh, install.sh, or shtool in ".." "../.." "../../.."

Checking the build-aux dir (created by autoreconf):

root@0546d5a47108:/tmp/aws-elasticache-cluster-client-libmemcached/build-aux# ls -l
total 424
-rwxr-xr-x 1 root root   7333 Jun 22 19:36 compile
-rwxr-xr-x 1 root root  42856 Jun 22 19:36 config.guess
-rwxr-xr-x 1 root root  35837 Jun 22 19:36 config.sub
-rwxr-xr-x 1 root root  23566 Jun 22 19:36 depcomp
-rwxr-xr-x 1 root root  14431 Jun 22 19:36 install-sh
-rw-r--r-- 1 root root 283672 Jun 22 19:36 ltmain.sh
-rwxr-xr-x 1 root root   6872 Jun 22 19:36 missing
-rwxr-xr-x 1 root root   4287 Jun 22 19:37 test-driver

@allyraza
Copy link

allyraza commented Aug 1, 2017

@raphaelquati you can symlink shtool into the dir you extracted the tar
configure: error: cannot find install-sh, install.sh, or shtool in ".." "../.." "../../.."

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