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

test/ec: build the libs only when 'make check' #7755

Closed
wants to merge 1 commit into from

Conversation

b-ranto
Copy link
Contributor

@b-ranto b-ranto commented Feb 23, 2016

Currently, we are always building the erasure code libraries while we
need them only when 'make check' is run. Moving the test libraries
to check_LTLIBRARIES should fix this for us.

See the autotools documentation on this for more details:

https://www.gnu.org/software/automake/manual/html_node/Libtool-Convenience-Libraries.html

So far, I've managed to build this in fedora and confirmed the libec_ test libs are gone. I'm not sure about the other part -- to check whether it did not break anything for 'make check'.

@b-ranto b-ranto changed the title test/erasure-code: build only when 'make check' [DNM] test/erasure-code: build only when 'make check' Feb 23, 2016
@b-ranto b-ranto changed the title [DNM] test/erasure-code: build only when 'make check' test/erasure-code: build only when 'make check' Feb 23, 2016
@b-ranto b-ranto changed the title test/erasure-code: build only when 'make check' test/ec: build the libs only when 'make check' Feb 23, 2016
@b-ranto
Copy link
Contributor Author

b-ranto commented Feb 23, 2016

The latest update just modified the commit message. This should supersede PR #7637.

@b-ranto b-ranto changed the title test/ec: build the libs only when 'make check' [DNM] test/ec: build the libs only when 'make check' Feb 23, 2016
@b-ranto b-ranto force-pushed the wip-ec-test-check branch 4 times, most recently from 010b248 to 3d61ed4 Compare February 25, 2016 19:07
@b-ranto
Copy link
Contributor Author

b-ranto commented Feb 25, 2016

It looks like I'm hitting mostly random fails jenkins runs. The latest one failed in test/osd/osd-markdown.sh which afaics should be in no relation to this patch.

As for the -rpath /nowhere part of the patch. I got it from here. I'm not entirely sure it is necessary for our purporse but it should not hurt anything. I'm currently waiting for the results of gitbuilders and fedora (koji) builds to see if the libec_test libs are indeed not being packaged.

@b-ranto
Copy link
Contributor Author

b-ranto commented Feb 25, 2016

This passed the gituilders, see the latest packages (there is no libec_test libs in the ceph-base package):

http://gitbuilder.ceph.com/ceph-rpm-centos7-x86_64-basic/ref/wip-ec-test-check/x86_64/

@b-ranto b-ranto changed the title [DNM] test/ec: build the libs only when 'make check' test/ec: build the libs only when 'make check' Feb 26, 2016
@liewegas liewegas assigned ghost Mar 1, 2016
@liewegas
Copy link
Member

liewegas commented Mar 1, 2016

loic, does this look ok?

@ghost
Copy link

ghost commented Mar 1, 2016

@BRANTO1 I don't think that works, IIRC the tests won't pass if we do that. That's because automake does different things when check_ is used instead of something_. I don't remember precisely why. However, if make check does work with that change, I'm all for it.

@b-ranto
Copy link
Contributor Author

b-ranto commented Mar 1, 2016

@dachary: Actually, the jenkins run says they passed

PASS: unittest_erasure_code_plugin
PASS: unittest_erasure_code
PASS: unittest_erasure_code_jerasure
PASS: unittest_erasure_code_plugin_jerasure
PASS: unittest_erasure_code_isa
PASS: unittest_erasure_code_plugin_isa
PASS: unittest_erasure_code_lrc
PASS: unittest_erasure_code_plugin_lrc
PASS: unittest_erasure_code_shec
PASS: test/encoding/readable.sh
PASS: unittest_erasure_code_shec_thread
PASS: unittest_erasure_code_shec_arguments
PASS: unittest_erasure_code_plugin_shec
PASS: unittest_erasure_code_example

but it is hitting a fail in

FAIL: test/osd/osd-markdown.sh

which looks like a time-out in the jenkins logs.

@smithfarm
Copy link
Contributor

I attempted to do this in #7701 using the same approach except without rpath= - make check did not pass. I will try running make check on this branch in my local environment.

@b-ranto
Copy link
Contributor Author

b-ranto commented Mar 1, 2016

@smithfarm: There is one crucial difference to your approach. Namely, I initialize the check_LTLIBRARIES in src/erasure-code/Makefile.am which (at least as far as I understand it) puts the libraries inside the src/erasure-code directory -- the directory that the unit tests are looking at for these libraries.

The -rpath bits might not actually be necessary at all and maybe, they are even the ones responsible for the random failures I'm hitting -- I ran it three times and I always hit one FAIL (per run) in one of the tests and each time, the test was outside the scope of the ec test libraries. Not sure what to think about it. Maybe, I was just unlucky.

@smithfarm
Copy link
Contributor

There is one crucial difference to your approach. Namely, I initialize the check_LTLIBRARIES in src/erasure-code/Makefile.am

Hmm. But I was doing this too - see 081bf1b

@b-ranto
Copy link
Contributor Author

b-ranto commented Mar 1, 2016

In that case, it might be the combination of all of those bits and the -rpath hack as described in

https://lists.gnu.org/archive/html/automake/2005-10/msg00107.html

@ghost
Copy link

ghost commented Mar 17, 2016

@BRANTO1 it looks like it needs rebasing or fixing to pass make check

@b-ranto b-ranto force-pushed the wip-ec-test-check branch 2 times, most recently from 2678360 to 29cab62 Compare March 17, 2016 16:41
@b-ranto
Copy link
Contributor Author

b-ranto commented Mar 17, 2016

@dachary rebased on top of latest jewel, we will see if it helps.

@ghost
Copy link

ghost commented Mar 29, 2016

@BRANTO1 it's worth another rebase, I think. But jewel is currently missing #8320 and fails make check so you want to wait before doing that ;-)

Currently, we are always building the erasure code libraries while we
need them only when 'make check' is run. Moving the test libraries to
check_LTLIBRARIES should fix this for us.

We no longer need to remove the libec libs manually, remove the lines
that do that.

Signed-off-by: Boris Ranto <branto@redhat.com>
@b-ranto
Copy link
Contributor Author

b-ranto commented Mar 30, 2016

@dachary Sure, thanks, done. Hopefully, it will pass jenkins, now.

@ghost
Copy link

ghost commented May 3, 2016

test this please

@ghost
Copy link

ghost commented May 3, 2016

@BRANTO1 we're past jewel now, would you mind re-targeting this to master ?

@ghost ghost closed this May 3, 2016
@b-ranto
Copy link
Contributor Author

b-ranto commented May 4, 2016

@dachary done in #8926 (it was a clean rebase)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants