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.

Sign up for GitHub

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

Jump to bottom

Fix 1.0.18 build issues #10

Closed
wants to merge 0 commits into from
Closed

Fix 1.0.18 build issues #10

wants to merge 0 commits into from

Conversation

jdolinski
Copy link

#9
#8
#7

Various changes for compilation and linking to fix build and pass. Tested on Ubuntu 14 and was able to complete all steps Outlined in the AWS documentation

Please review, I have not worked with Makefiles and open to any feedback.

@jdolinski
Copy link
Author

@kmaleyev Also, confirmed build and compile completed on Amazon Linux AMI 2017.03.0.
Let me know if you have any feedback. Thanks!

@malyeyev-AMZN
Copy link
Contributor

Thank you for the contribution!

Before this can be merged in mainline I would need to test it on multiple platforms. I will merge this into 'autoconf' branch today and hopefully find time to get it tested and merge it into the mainline sometime in the near future. Please let me know if this sounds good.

@malyeyev-AMZN
Copy link
Contributor

Jim,

Thanks again for all this work. This sounds like a hugely frustrating experience and I want to make sure other users don't have to got through this.

General comments on this pull request (there's a separate review with technical inline comments):

  1. So, unfortunately, I'm not a big expert on Autotools either. My understanding is we made a bit of mess initially when forking this project either by modifying Makefile.in instead of the real source files (Makefile.am and configure.ac) or by which files we checked in the repository. I'm not quite sure what it is exactly but I think this SO question would be a good pointer: http://stackoverflow.com/questions/4103349/how-configure-script-decides-to-regenerate-itself. Until we actually understand what's going on, I would be reluctant to merge any fixes into mainline.

  2. Confirming I was able to successfully build this code on Ubuntu 14:

# Ubuntu SERVER 14.04 LTS
# ubuntu/images/ebs/ubuntu-trusty-14.04-amd64-server-20170307-ecd5575e-d805-450e-843e-f2a9872b8c80-ami-9fde7f89.4 (ami-793d9e6f)

sudo apt-get update
sudo apt-get install git
git clone https://github.com/jdolinski/aws-elasticache-cluster-client-libmemcached.git
cd aws-elasticache-cluster-client-libmemcached/
sudo apt-get install libevent-dev gcc g++ make autoconf libsasl2-dev
autoreconf -vfi
mkdir BUILD
cd BUILD
../configure --prefix=/home/ubuntu/aws-elasticache-cluster-client-libmemcached/local --with-pic
make

  1. So if you're suggesting to run autoreconf, why still include 'Makefile.in' and 'configure' (which both will be re-generated by it from Makefile.am and configure.ac) in the source distribution?

  2. You might have forgotten to configure git with your name/email until the later commits: Author: root <[email protected]>, probably also want to squash those commits into one or a few, whichever makes sense.

malyeyev-AMZN
malyeyev-AMZN suggested changes Apr 11, 2017
View reviewed changes
tests/libmemcached-1.0/dynamic_mode_test.cc Outdated
@@ -436,7 +436,7 @@ test_return_t polling_test(memcached_st *ptr)

#include "tests/libmemcached-1.0/dynamic_mode_test.h"

void get_world(libtest::Framework *world)
void get_world_dynamic_mode(libtest::Framework *world)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see this being referenced from anywhere.

grep -rnI get_world_dynamic_mode *
tests/libmemcached-1.0/dynamic_mode_test.cc:439:void get_world_dynamic_mode(libtest::Framework *world)

On top of that these tests are not part of the default build test or build (release) targets and are for Amazon's internal use only, so you should have no need to touch them.

Copy link
Author

Choose a reason for hiding this comment

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

This was causing compilation issues and duplicate error. See note on my PR #11. I ended up leaving dynamic_mode_test alone and modified the all_tests. These two files were causing duplicate function errors during compilation.

README.md Outdated

> sudo apt-get install libevent-dev gcc g++ make autoconf libsasl2-dev

c) Check Automake Version

> sudo automake --version
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] no need for sudo i think, same for autoreconf -vfi below

Copy link
Author

Choose a reason for hiding this comment

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

I had to use sudo on both ubuntu and AWS AMI

README.md Outdated

> If you are not running version 1.14.x you will need to update `<root>/configure` file's variable `am__api_version='1.14'` to match your version of automake.

> sudo autoreconf -vfi (This will [remake build system files](http://www.tutorialspoint.com/unix_commands/autoreconf.htm) in the directory trees driven by configure.ac)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be moved down after git clone in step (3)?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, no need to check your version of automake. Removed the configure and Makefine.in and when autoreconf is run it will regenerate the files based upon the version of automake installed on the machine.

tests/libmemcached-1.0/all_tests.h Outdated
@@ -443,7 +443,7 @@ test_st namespace_tests[] ={
{0, 0, (test_callback_fn*)0}
};

collection_st collection[] ={
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] Unless I'm missing something this seems unnecessary. Wouldn't we want to leave it the way it is to be consistent with all the other test definition files?

Copy link
Author

Choose a reason for hiding this comment

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

This was causing compilation issues and duplicate error. See note on my PR #11

@jdolinski
Copy link
Author

Thanks for the great feedback:) Let me digest, clean things up, and hopefully clarify some of your questions. This was a great learning experience working with this project. I typically work with Java and its build systems. I still have PR for a open source CMS to incorporate the autodiscovery...This is not urgent but will get back to you soon.

@jdolinski jdolinski closed this Apr 13, 2017
@jdolinski
Copy link
Author

See PR #11

@jdolinski jdolinski mentioned this pull request Apr 13, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malyeyev-AMZN malyeyev-AMZN malyeyev-AMZN requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdolinski @malyeyev-AMZN