Removed unnecessary inclusion of iostream in several files #4707

stiopaa1 · 2015-05-17T13:08:57Z

Signed-off-by: Michal Jarzabek stiopa@gmail.com

loic-bot · 2015-05-17T13:20:03Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/5281/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5281/

ghost · 2015-05-17T13:53:04Z

CXXLD    ceph_test_rados_striper_api_striping
/usr/bin/ld: ./.libs/libcommon.a(Crypto.o): undefined reference to symbol 'PK11_GetBestSlot@@NSS_3.2'
/usr/bin/ld: note: 'PK11_GetBestSlot@@NSS_3.2' is defined in DSO /usr/lib/gcc/x86_64-redhat-linux/4.8.3/../../../../lib64/libnss3.so so try adding it to the linker command line
/usr/lib/gcc/x86_64-redhat-linux/4.8.3/../../../../lib64/libnss3.so: could not read symbols: Invalid operation

You should be able to reproduce this error on CentOS 7. If you have docker you can setup an environment with:

cd src ; test/docker-test.sh --os-type centos --os-version 7 --shell

stiopaa1 · 2015-05-17T18:07:57Z

@dachary

cd src ; test/docker-test.sh --os-type centos --os-version 7 --shell

This still seem to build fine on my machine (assuming that I've done it correctly).

ghost · 2015-05-17T19:13:44Z

@stiopaa1 would you mind doing git commit --amend (just for the purpose of changing the hash of your last commit) followed by a git push --force (just for the sake of running the bot again). If the bot shows the exact same error, it will be helpful if you can upload the output of your run.

Just to avoid any misunderstanding, when you run cd src ; test/docker-test.sh --os-type centos --os-version 7 --shell it just gives you a shell on a centos machine, with your latest commit, in the ceph tree. From there it's up to you to ./run-make-check.sh. Is it what you did ?

stiopaa1 · 2015-05-17T19:21:32Z

@dachary after running cd src ; test/docker-test.sh --os-type centos --os-version 7 --shell, I did(in ceph-centos-7-stiopa directory):
./autogen.sh
./configure
make -j 8
make check

I'll give it another go with ./run-make-check.sh

loic-bot · 2015-05-17T19:27:19Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/5292/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5292/

ghost · 2015-05-17T19:37:09Z

@stiopaa1 the ./run-make-check.sh script is run by the bot and does not use the same compilation flag. It's likely you will run into the same problem.

ghost · 2015-05-17T19:38:03Z

http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5292/console

  CXXLD    ceph_test_rados_api_c_read_operations
/usr/bin/ld: ./.libs/libcommon.a(Crypto.o): undefined reference to symbol 'PK11_GetBestSlot@@NSS_3.2'
/usr/bin/ld: note: 'PK11_GetBestSlot@@NSS_3.2' is defined in DSO /usr/lib/gcc/x86_64-redhat-linux/4.8.3/../../../../lib64/libnss3.so so try adding it to the linker command line
/usr/lib/gcc/x86_64-redhat-linux/4.8.3/../../../../lib64/libnss3.so: could not read symbols: Invalid operation
collect2: error: ld returned 1 exit status

stiopaa1 · 2015-05-17T20:07:19Z

@dachary
so after running ./run-make-check.sh i'm getting:
configure: error: C compiler cannot create executables

here is the full log:
http://hastebin.com/qapojepuqu.coffee

smithfarm · 2015-05-17T20:19:59Z

Just noticed in the pasted log:

    Installed /tmp/easy_install-uQ8jd5/linecache2-1.0.0/pbr-0.11.0-py2.7.egg
      File "build/bdist.linux-x86_64/egg/linecache2/tests/inspect_fodder2.py", line 102
        def keyworded(*arg1, arg2=1):
                                ^
    SyntaxError: invalid syntax

      File "/home/stiopa/development/cephStiopa/ceph-centos-7-stiopa/install-deps-python2.7/build/unittest2/linecache2-1.0.0-py2.7.egg/linecache2/tests/inspect_fodder2.py", line 102
        def keyworded(*arg1, arg2=1):
                                ^
    SyntaxError: invalid syntax

ghost · 2015-05-17T20:53:00Z

http://hastebin.com/qapojepuqu.coffee

the config.log file should have more information about why it failed.

stiopaa1 · 2015-05-20T21:06:06Z

@dachary, thanks I managed to reproduce it locally - hopefully will work now:)

loic-bot · 2015-05-20T21:46:01Z

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/5337/

ghost · 2015-05-20T21:55:28Z

@stiopaa1 that's great. It makes me worried there could problems with other compilation flags. What was the problem ?

stiopaa1 · 2015-05-25T21:19:08Z

@dachary

It makes me worried there could problems with other compilation flags

The flag that made it fail was --with-debug - without it the file(src/test/cls_rbd/test_cls_rbd.cc) causing the problem wasn't being compiled.

What was the problem ?

I think(!) that following happened:
The problem was being caused by moving the inline functions from buffer.h to buffer.cc.

The following line was causing the error:
/bin/bash ../libtool --tag=CXX --mode=link g++ -I/usr/include/nss -I/usr/include/nspr -Wall -Wtype-limits -Wignored-qualifiers -Winit-self -Wpointer-arith -Werror=format-security -fno-strict-aliasing -fsigned-char -rdynamic -ftemplate-depth-1024 -Wnon-virtual-dtor -Wno-invalid-offsetof -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -Wstrict-null-sentinel -I../src/gmock/include -I../src/gmock/include -I../src/gmock/gtest/include -I../src/gmock/gtest/include -g -O2 -Wl,--as-needed -latomic_ops -o ceph_test_cls_rbd test/cls_rbd/ceph_test_cls_rbd-test_cls_rbd.o librados.la libcls_rbd_client.la libcls_lock_client.la libcommon.la ../src/gmock/lib/libgmock_main.la ../src/gmock/lib/libgmock.la ../src/gmock/gtest/lib/libgtest.la -lpthread libradostest.la -lboost_system

Before the inline functions were moved to buffer.cc, the above didnt't require libcommon library and even though the library is listed it wasn't being linked against. When we move inline functions to buffer.cc(and remove inline) the libcommon is linked against, but is then missing libnss3 library(in Crypto.o), hence the error:

/usr/bin/ld: ./.libs/libcommon.a(Crypto.o): undefined reference to symbol 'PK11_GetBestSlot@@NSS_3.2'
/usr/bin/ld: note: 'PK11_GetBestSlot@@NSS_3.2' is defined in DSO /usr/lib/gcc/x86_64-redhat-linux/4.8.3/../../../../lib64/libnss3.so so try adding it to the linker command line
/usr/lib/gcc/x86_64-redhat-linux/4.8.3/../../../../lib64/libnss3.so: could not read symbols: Invalid operation
collect2: error: ld returned 1 exit status

ghost · 2015-05-25T21:54:24Z

that makes sense. It also deserves explaining in the commit message. Also the main commit does not just remove unused iostream. It deserves a detailed explanation of why it is a good thing. Why moving functions from buffer.h to buffer.cc is beneficial ?

ghost · 2015-05-25T22:03:43Z

And the two commits can be squashed together I think. Apart from the commit message the change looks great.

ghost · 2015-05-25T22:06:02Z

I think the most important thing to explain is why performances won't be negatively affected by having the functions not inline anymore.

stiopaa1 · 2015-05-25T22:21:47Z

@dachary
Hope that's enough:)

ghost · 2015-05-25T22:49:00Z

@stiopaa1 buffer.cc being used extensively throughout the code base, there needs to be certainty that it won't affect performances. Can you make an extra effort to verify and explain ? I think you're right and it would probably not matter as much if it was not in such a central place.

ghost · 2015-05-25T22:49:55Z

@stiopaa1 the rest of the explaining is good enough. I even think you should merge the two commits together because the first won't even link without the second.

loic-bot · 2015-05-25T23:00:35Z

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/5408/

loic-bot · 2015-05-25T23:21:50Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/5409/

gregsfortytwo · 2015-06-03T06:23:43Z

Okay, you can shepherd it if that's an issue. :)

stiopaa1 · 2015-06-03T20:42:09Z

@stiopaa1 probably we can add $(CRYPTO_LIBS) and $(EXTRALIBS) instead of$(CEPH_GLOBAL). but i am also fine if you want to stick with the later, as you may already noticed, some other tests are also linking agaist it

@tchaikov I have replaced CEPH_GLOBAL with CRYPTO_LIBS and EXTRALIBS as suggested.

loic-bot · 2015-06-03T22:10:40Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/5651/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5651/

loic-bot · 2015-06-06T20:41:03Z

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/5719/

tchaikov · 2015-06-08T03:11:48Z

@stiopaa1 thank you Michal! i am pushing your changes to ceph.git/wip-4707. once it is confirmed by the builders, will merge it.

tchaikov · 2015-06-08T04:07:58Z

@stiopaa1 , the build failed at http://gitbuilder.sepia.ceph.com/gitbuilder-ceph-rpm-centos6-5-amd64-basic/log.cgi?log=68c3ac2bef13214263bf7b47567978e8ceb52249

CXX test/osd/Object.o
CXX test/osd/RadosModel.o
test/osd/Object.cc: In member function 'bool ObjectDesc::check(ceph::bufferlist&)':
error: test/osd/Object.cc:169: 'cout' is not a member of 'std'
error: test/osd/Object.cc:173: 'cout' is not a member of 'std'
error: test/osd/Object.cc:180: 'cout' is not a member of 'std'

Michal, could you also #include <iostream> in test/osd/Object.cc to fix this also? thanks =)

loic-bot · 2015-06-09T01:42:22Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/5753/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5753/

In several files the iostream wasn't being used, so it got removed. In other files the iostream inclusion was replaced by including iosfwd (for forward declarations), which is much smaller header than iostream, so in theory should reduce compilation time. To make this work some of the functions must have been moved from .h to .cc file. 3 functions also needed to have inline removed - this shouldn't affect performance in any way: two of them are probably too long to have been inlined anyway and the third one is for error reporting, so probably won't be called too often. test/Makefile-client.am: added linker libs This was required to avoid linker error when linking src/test/cls_rbd/test_cls_rbd.cc file. Makefile was specyfing libcommon.a as a part of a linker command even though this wasn't required and wasn't being linked against. When inline functions from buffer.h were moved to buffer.cc(and inline was removed) the libcommon.a library became necessary. This wouldn't link without also including additional libraries(CRYPTO_LIBS and EXTRA_LIBS) Signed-off-by: Michal Jarzabek <stiopa@gmail.com>

loic-bot · 2015-06-09T08:01:19Z

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/5760/

FAILURE http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5760/

loic-bot · 2015-06-09T08:57:04Z

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/5761/

stiopaa1 · 2015-06-09T16:02:23Z

@tchaikov
I've included iostream in Object.cc

Any idea why the build first failed and then succeeded?

ghost · 2015-06-09T16:19:45Z

@tchaikov @stiopaa1 you can ignore #4707 (comment) this is a false negative

Removed unnecessary inclusion of iostream in several files Reviewed-by: Kefu Chai <kchai@redhat.com>

fix xio to work with the merge of pull request " Removed unnecessary inclusion of iostream in several files ceph#4707" Signed-off-by: Vu Pham <vu@mellanox.com>

fix xio to work with the merge of pull request " Removed unnecessary inclusion of iostream in several files #4707" Signed-off-by: Vu Pham <vu@mellanox.com>

fix xio to work with the merge of pull request " Removed unnecessary inclusion of iostream in several files ceph#4707" Signed-off-by: Vu Pham <vu@mellanox.com>

stiopaa1 mentioned this pull request May 17, 2015

Removed unnecessary inclusion of iostream in several files #4647

Closed

ghost added cleanup core labels May 17, 2015

stiopaa1 force-pushed the IosFwdNew branch from eebe1ed to c85ce9d Compare May 17, 2015 19:19

ghost self-assigned this May 25, 2015

stiopaa1 force-pushed the IosFwdNew branch from ccbea31 to 5ba3428 Compare May 25, 2015 22:20

stiopaa1 force-pushed the IosFwdNew branch from 5ba3428 to 3bee6fa Compare May 25, 2015 22:59

tchaikov reopened this Jun 3, 2015

gregsfortytwo assigned tchaikov and unassigned gregsfortytwo Jun 3, 2015

stiopaa1 force-pushed the IosFwdNew branch from 3bee6fa to bbe610d Compare June 3, 2015 20:40

stiopaa1 force-pushed the IosFwdNew branch 3 times, most recently from a213f5e to 68c3ac2 Compare June 6, 2015 19:13

stiopaa1 force-pushed the IosFwdNew branch from 68c3ac2 to 52bd944 Compare June 8, 2015 18:59

stiopaa1 force-pushed the IosFwdNew branch from 52bd944 to 83f8198 Compare June 9, 2015 06:27

tchaikov added a commit that referenced this pull request Jun 10, 2015

Merge pull request #4707 from stiopaa1/IosFwdNew

5a5dc5f

Removed unnecessary inclusion of iostream in several files Reviewed-by: Kefu Chai <kchai@redhat.com>

tchaikov merged commit 5a5dc5f into ceph:master Jun 10, 2015

vuhuong mentioned this pull request Jun 12, 2015

xio - work with accelio v1.4 #4946

Closed

vuhuong mentioned this pull request Jun 23, 2015

xio: sync works #5064

Merged

tchaikov pushed a commit that referenced this pull request Jun 24, 2015

xio: fix to work with the merge of pull request #4707

47a8447

fix xio to work with the merge of pull request " Removed unnecessary inclusion of iostream in several files #4707" Signed-off-by: Vu Pham <vu@mellanox.com>

ghost mentioned this pull request Jun 27, 2015

common/Thread: cleanup #5099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed unnecessary inclusion of iostream in several files #4707

Removed unnecessary inclusion of iostream in several files #4707

stiopaa1 commented May 17, 2015

loic-bot commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 17, 2015

loic-bot commented May 17, 2015

ghost commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 17, 2015

smithfarm commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 20, 2015

loic-bot commented May 20, 2015

ghost commented May 20, 2015

stiopaa1 commented May 25, 2015

ghost commented May 25, 2015

ghost commented May 25, 2015

ghost commented May 25, 2015

stiopaa1 commented May 25, 2015

ghost commented May 25, 2015

ghost commented May 25, 2015

loic-bot commented May 25, 2015

loic-bot commented May 25, 2015

gregsfortytwo commented Jun 3, 2015

stiopaa1 commented Jun 3, 2015

loic-bot commented Jun 3, 2015

loic-bot commented Jun 6, 2015

tchaikov commented Jun 8, 2015

tchaikov commented Jun 8, 2015

loic-bot commented Jun 9, 2015

loic-bot commented Jun 9, 2015

loic-bot commented Jun 9, 2015

stiopaa1 commented Jun 9, 2015

ghost commented Jun 9, 2015

Removed unnecessary inclusion of iostream in several files #4707

Removed unnecessary inclusion of iostream in several files #4707

Conversation

stiopaa1 commented May 17, 2015

loic-bot commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 17, 2015

loic-bot commented May 17, 2015

ghost commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 17, 2015

smithfarm commented May 17, 2015

ghost commented May 17, 2015

stiopaa1 commented May 20, 2015

loic-bot commented May 20, 2015

ghost commented May 20, 2015

stiopaa1 commented May 25, 2015

ghost commented May 25, 2015

ghost commented May 25, 2015

ghost commented May 25, 2015

stiopaa1 commented May 25, 2015

ghost commented May 25, 2015

ghost commented May 25, 2015

loic-bot commented May 25, 2015

loic-bot commented May 25, 2015

gregsfortytwo commented Jun 3, 2015

stiopaa1 commented Jun 3, 2015

loic-bot commented Jun 3, 2015

loic-bot commented Jun 6, 2015

tchaikov commented Jun 8, 2015

tchaikov commented Jun 8, 2015

loic-bot commented Jun 9, 2015

loic-bot commented Jun 9, 2015

loic-bot commented Jun 9, 2015

stiopaa1 commented Jun 9, 2015

ghost commented Jun 9, 2015