ZOOKEEPER-3436 Enhance Mavenized Make C client #993

eolivelli · 2019-06-20T18:18:04Z

Fix the pom of the zookeeeper-c-client in order to comply to the standard maven workflow:
Build without tests:
mvn clean install -DskipTests
Build and test
mvn clean install -DskipTests

More details here:
https://issues.apache.org/jira/browse/ZOOKEEPER-3436

anmolnar

+1 Flawless on Centos 7.

phunt · 2019-06-21T00:25:00Z

Fails for me with

mvn clean install -DskipTests -Pfull-build

"ant clean compile compile-native" works fine with the same codebase.

Here's the error I see with the mvn build attempt:

 [exec] libtool: link: gcc -dynamiclib  -o .libs/libzookeeper_st.2.dylib   -Wl,-force_load,./.libs/libzkst.a -Wl,-force_load,./.libs/libhashtable.a  -lm -lgcov  -fprofile-arcs -g -O2   -install_name  /Users/phunt/dev/zookeeper-trunk/zookeeper-client/zookeeper-client-c/target/c/lib/libzookeeper_st.2.dylib -compatibility_version 3 -current_version 3.0 -Wl,-single_module -Wl,-exported_symbols_list,.libs/libzookeeper_st-symbols.expsym
 [exec] ld: library not found for -lgcov
 [exec] clang: error: linker command failed with exit code 1 (use -v to see invocation)
 [exec] make: *** [libzookeeper_st.la] Error 1

phunt

Seems to be failing with maven and not ant.

eolivelli · 2019-06-21T09:11:21Z

It is failing for me as well in another env.
I guess it is releated to garbage files.
We have a lot of stuff in .gitignore

I am cleaning up

eolivelli · 2019-06-21T11:27:03Z

@phunt I solved my problem by cleaning up all stuff generated by previous build with ant (mostly like these ones, and all .so, .o....) :

        Makefile.in
	aclocal.m4
	autom4te.cache/
	compile
	config.guess
	config.h.in
	config.sub
	configure
	depcomp
	install-sh
	ltmain.sh
	missing

eolivelli · 2019-06-21T11:28:21Z

I could add in a "clean" phase some sort of purge task that clean every artifact produced by "make" (with a static list)

eolivelli · 2019-06-21T11:39:09Z

I have committed a more maven-friendly .gitignore for zookeeper-c-client directory

@phunt @anmolnar You should see the file to be deleted with "git status" as files produced by a direct execution of commands on the zookeeper-c-client directory

eolivelli · 2019-06-21T13:24:02Z

C client tests passed on ASF Jenkins
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/845/org.apache.zookeeper$zookeeper-client-c/console

They run on my Fedora laptop as well

nkalmar

+1 from my side, tested after git clean -fdx and ant clean, with multiple runs including
mvn clean install -DskipTests -Pfull-build

Ubuntu 16.04.6 LTS (Xenial Xerus)

Let's wait and see if the second commit solves for Patrick as well.

anmolnar · 2019-06-21T21:28:21Z

ant clean / mvn clean has never completely cleaned configure/make leftovers, I always had to git clean -fxd before testing (like Norbert did). So it's not a regression for me, however it'd nice to fix this too.

eolivelli · 2019-06-23T19:56:59Z

@phunt shall we merge this patch as it is or would you prefer that I enhance the 'clean' task?

eolivelli · 2019-07-26T21:40:03Z

@phunt ping

eolivelli · 2019-08-01T05:26:19Z

@phunt can we merge this patch ?

cc @anmolnar @nkalmar

anmolnar · 2019-08-02T12:17:39Z

Cannot be merged until @phunt removes the -1.

eolivelli · 2019-08-15T17:29:52Z

@phunt PTAL

eolivelli · 2019-08-25T06:07:00Z

@phunt I see you are online, please take a look.

phunt · 2019-09-03T02:12:36Z

Afraid I'm still seeing the same issue with mvn while ant works fine, this is mvn:

 [exec] libtool: link: gcc -dynamiclib  -o .libs/libzookeeper_st.2.dylib   -Wl,-force_load,./.libs/libzkst.a -Wl,-force_load,./.libs/libhashtable.a  -lm -lgcov  -fprofile-arcs -g -O2   -install_name  /Users/phunt/dev/zookeeper-trunk/zookeeper-client/zookeeper-client-c/target/c/lib/libzookeeper_st.2.dylib -compatibility_version 3 -current_version 3.0 -Wl,-single_module -Wl,-exported_symbols_list,.libs/libzookeeper_st-symbols.expsym
 [exec] ld: library not found for -lgcov
 [exec] clang: error: linker command failed with exit code 1 (use -v to see invocation)
 [exec] make: *** [libzookeeper_st.la] Error 1

I did a "git clean -xdf" but that had/has no effect.

eolivelli · 2019-09-03T06:49:35Z

Thank you @phunt for checking again.
I don't know it, but gcov is a code coverage library.
I will try to check for it in the ant script.

@nkalmar @symat I appreciate very much if you have cycles to help me, this C part is very far from my usual work.

symat · 2019-09-03T08:10:26Z

we found the problem with @nkalmar:

the gcov library is used only when the --enable-gcov parameter is passed to the configure call. This parameter was used in the ant file only for the target c_coverage_report but not during the normal test / build targets. Currently in maven it is hardcoded to be always passed to the configure script. And most probably the gcov library is not present on Patrick's machine.

(https://github.com/eolivelli/zookeeper/blob/44f6ba5d2530d3376e469524df0b946b77a69fcb/zookeeper-client/zookeeper-client-c/pom.xml#L74)

It would be nice to introduce an optional maven parameter and not using gcov by default. (It doesn't make much sense to measure test coverage all the time when we want to build the client).

nkalmar · 2019-09-03T08:35:47Z

zookeeper-client/zookeeper-client-c/pom.xml

          <execution>
            <id>configure</id>
-            <phase>test-compile</phase>
+            <phase>process-sources</phase>


This is where --enable-gcov is passed, the phase which this runs probably surfaced the problem for Patrick? If someone doesn't have the library it will always fail, with ant only if they run a c-coverege report.
Thanks @symat for finding this!

nkalmar · 2019-09-03T08:37:47Z

Sorry @eolivelli , this is my bad, during the migration I didn't pay attention gcov should only be passed if we run code-coverage.

(As I had the library installed it never failed for me)

eolivelli · 2019-09-03T09:49:48Z

Can you pick up this branch?

symat · 2019-09-03T10:37:04Z

Can you pick up this branch?

do you suggest to create a new Jira about adding a new maven parameter for --enable-gcov ?
I am happy to do that, and I think this PR could be merged then.

eolivelli · 2019-09-03T10:58:52Z

@symat I think you can do it within this JIRA, it is about "Enhance the Maven build".
You can continue from this branch.
Feel free to open a new JIRA if you are able to split the two activities.
To me the important goal is that is is easy to work on the C client without ant.
The overall goal is to drop the ant based support and this problem is a real showstopper.

I will review your patch as soon as it is ready

Thanks

symat · 2019-09-04T08:12:44Z

I created a new PR (#1078), please take a look.

eolivelli · 2019-09-04T09:22:48Z

closing this one, thank you @symat

Based on eolivelli 's [previous pull request](#993) we enhanced now the maven C client build: - we can compile now the C-client without tests (using the -DskipTests option) - we can optionally enable the test coverage calculation for the C-client (-Pc-test-coverage) - we also package the C-client into the binary tarball (when -Pfull-build is used) - I also updated the README_packaging.md file to make user's life easier (happier? :) ) I tested the build on ubuntu 16.04. Author: Mate Szalay-Beko <mszalay@cloudera.com> Author: Enrico Olivelli <eolivelli@apache.org> Reviewers: nkalmar@apache.org, eolivelli@apache.org, andor@apache.org Closes #1078 from symat/ZOOKEEPER-3436 and squashes the following commits: 18250df [Mate Szalay-Beko] ZOOKEEPER-3436 remove the C binaries from the binary tarball; update the readme files 57b7cd6 [Mate Szalay-Beko] ZOOKEEPER-3436 fixing file permissions of usr/bin/* in binary tarball a2ac025 [Mate Szalay-Beko] ZOOKEEPER-3436 enhance / format README_packaging.md 30eea5e [Mate Szalay-Beko] ZOOKEEPER-3436 exclude README_packaging.md from license check 936550e [Mate Szalay-Beko] ZOOKEEPER-3436 fixing README_packaging.md f010295 [Mate Szalay-Beko] ZOOKEEPER-3436 Enhance Mavenized Make C client 44f6ba5 [Enrico Olivelli] Better .gitignore 2046037 [Enrico Olivelli] Enhance Mavenized Make C client

Based on eolivelli 's [previous pull request](apache#993) we enhanced now the maven C client build: - we can compile now the C-client without tests (using the -DskipTests option) - we can optionally enable the test coverage calculation for the C-client (-Pc-test-coverage) - we also package the C-client into the binary tarball (when -Pfull-build is used) - I also updated the README_packaging.md file to make user's life easier (happier? :) ) I tested the build on ubuntu 16.04. Author: Mate Szalay-Beko <mszalay@cloudera.com> Author: Enrico Olivelli <eolivelli@apache.org> Reviewers: nkalmar@apache.org, eolivelli@apache.org, andor@apache.org Closes apache#1078 from symat/ZOOKEEPER-3436 and squashes the following commits: 18250df [Mate Szalay-Beko] ZOOKEEPER-3436 remove the C binaries from the binary tarball; update the readme files 57b7cd6 [Mate Szalay-Beko] ZOOKEEPER-3436 fixing file permissions of usr/bin/* in binary tarball a2ac025 [Mate Szalay-Beko] ZOOKEEPER-3436 enhance / format README_packaging.md 30eea5e [Mate Szalay-Beko] ZOOKEEPER-3436 exclude README_packaging.md from license check 936550e [Mate Szalay-Beko] ZOOKEEPER-3436 fixing README_packaging.md f010295 [Mate Szalay-Beko] ZOOKEEPER-3436 Enhance Mavenized Make C client 44f6ba5 [Enrico Olivelli] Better .gitignore 2046037 [Enrico Olivelli] Enhance Mavenized Make C client

eolivelli requested review from nkalmar and phunt June 20, 2019 18:18

eolivelli mentioned this pull request Jun 20, 2019

ZOOKEEPER-3433 zkpython build broken after maven migration #991

Closed

anmolnar approved these changes Jun 20, 2019

View reviewed changes

phunt requested changes Jun 21, 2019

View reviewed changes

nkalmar approved these changes Jun 21, 2019

View reviewed changes

eolivelli changed the title ~~Enhance Mavenized Make C client~~ ZOOKEEPER-3436 Enhance Mavenized Make C client Sep 2, 2019

symat mentioned this pull request Sep 2, 2019

ZOOKEEPER-3530 Include compiled C-client in the binary tarball #1074

Closed

eolivelli added 2 commits September 3, 2019 09:04

Enhance Mavenized Make C client

2046037

Better .gitignore

44f6ba5

eolivelli force-pushed the fix/ZOOKEEPER-3436-c-client branch from 29384f2 to 44f6ba5 Compare September 3, 2019 07:05

nkalmar reviewed Sep 3, 2019

View reviewed changes

symat mentioned this pull request Sep 4, 2019

ZOOKEEPER-3436 Enhance Mavenized Make C client #1078

Closed

eolivelli closed this Sep 4, 2019

ZOOKEEPER-3436 Enhance Mavenized Make C client #993

ZOOKEEPER-3436 Enhance Mavenized Make C client #993

Uh oh!

Conversation

eolivelli commented Jun 20, 2019

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

phunt commented Jun 21, 2019

Uh oh!

phunt left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Jun 21, 2019

Uh oh!

eolivelli commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli commented Jun 21, 2019

Uh oh!

eolivelli commented Jun 21, 2019

Uh oh!

eolivelli commented Jun 21, 2019

Uh oh!

nkalmar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli commented Jun 23, 2019

Uh oh!

eolivelli commented Jul 26, 2019

Uh oh!

eolivelli commented Aug 1, 2019

Uh oh!

anmolnar commented Aug 2, 2019

Uh oh!

eolivelli commented Aug 15, 2019

Uh oh!

eolivelli commented Aug 25, 2019

Uh oh!

phunt commented Sep 3, 2019

Uh oh!

eolivelli commented Sep 3, 2019

Uh oh!

symat commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkalmar Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

nkalmar commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli commented Sep 3, 2019

Uh oh!

symat commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli commented Sep 3, 2019

Uh oh!

symat commented Sep 4, 2019

Uh oh!

eolivelli commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eolivelli commented Jun 21, 2019 •

edited

Loading

nkalmar left a comment •

edited

Loading

anmolnar commented Jun 21, 2019 •

edited

Loading

symat commented Sep 3, 2019 •

edited

Loading

nkalmar commented Sep 3, 2019 •

edited

Loading

symat commented Sep 3, 2019 •

edited

Loading