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

build/ops: conditionalize rgw Beast frontend so it isn't built on s390x architecture #15225

Merged
merged 6 commits into from May 30, 2017

Conversation

Projects
None yet
6 participants
@smithfarm
Contributor

smithfarm commented May 23, 2017

Fix for http://tracker.ceph.com/issues/20048

Problem solved by this PR: boost::component is not supported on s390x architecture, breaking the s390x build.

Also, it's a Good Idea in general to tie dependencies to the code that actually uses them.

(N.B.: Ceph is being built on s390x only so Linux VMs on the mainframe can function as Ceph clients.)

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 23, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 23, 2017

Confirmed that this doesn't break any of the upstream builds. Now trying to get it to build successfully on s390x.

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 23, 2017

can you paste your build/CMakeFiles/CMakeError.log when it fails to find boost context?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 23, 2017

@cbodley I can try, but I currently have no way to access the remote build environment where these s390x builds are taking place.

set(BOOST_HEADER_COMPONENTS container)
if(WITH_MGR)
list(APPEND BOOST_COMPONENTS python)
endif()
if(WITH_RADOSGW)

This comment has been minimized.

@cbodley

cbodley May 23, 2017

Contributor

if (WITH_RADOSGW_BEAST_FRONTEND) should be sufficient here. then you don't need to change the packaging for radosgw

@smithfarm smithfarm changed the title from [DNM] Conditionalize RGW build to build/ops: conditionalize rgw Beast build May 24, 2017

@smithfarm smithfarm changed the title from build/ops: conditionalize rgw Beast build to build/ops: conditionalize rgw Beast so it isn't built on s390x architecture May 24, 2017

@mdw-at-linuxbox

This comment has been minimized.

Contributor

mdw-at-linuxbox commented May 24, 2017

Not being able to see error logs from s390x seems to me to be a problem in itself.

Also it would be nice to know more about why this was a problem on s390x - does this mean we have to avoid using boost::context everywhere else in ceph too?

@tserong

This comment has been minimized.

Contributor

tserong commented May 24, 2017

Not being able to see error logs from s390x seems to me to be a problem in itself.

That's because it's built in a build farm, where users don't have access to the insides of the VMs where the builds are done.

Also it would be nice to know more about why this was a problem on s390x - does this mean we have to avoid using boost::context everywhere else in ceph too?

Apparently boost::context isn't supported on s390x. From SUSE#114 (comment):

[  121s] libs/context/src/unsupported.cpp:7:2: error: #error "platform not supported"
[  121s]  #error "platform not supported"
[  121s]   ^
[  164s] ...skipped <p/home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495217527.9b626e3f69/build/boost-build/boost/bin.v2/libs/context/build/gcc-4.8/release/link-static/threading-multi>libboost_context.a(clean) for lack of <p/home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495217527.9b626e3f69/build/boost-build/boost/bin.v2/libs/context/build/gcc-4.8/release/link-static/threading-multi>unsupported.o...
[  164s] ...skipped <p/home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495217527.9b626e3f69/build/boost-build/boost/bin.v2/libs/context/build/gcc-4.8/release/link-static/threading-multi>libboost_context.a for lack of <p/home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495217527.9b626e3f69/build/boost-build/boost/bin.v2/libs/context/build/gcc-4.8/release/link-static/threading-multi>unsupported.o...
[  164s] ...skipped <p/home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495217527.9b626e3f69/build/boost/lib>libboost_context.a for lack of <p/home/abuild/rpmbuild/BUILD/ceph-12.0.3+git.1495217527.9b626e3f69/build/boost-build/boost/bin.v2/libs/context/build/gcc-4.8/release/link-static/threading-multi>libboost_context.a...
[  196s] ...failed updating 1 target...
[  196s] CMake Warning at /usr/share/cmake/Modules/FindBoost.cmake:725 (message):
[  196s]   Imported targets not available for Boost version 106300
[  196s] Call Stack (most recent call first):
[  196s]   /usr/share/cmake/Modules/FindBoost.cmake:763 (_Boost_COMPONENT_DEPENDENCIES)
[  196s]   /usr/share/cmake/Modules/FindBoost.cmake:1315 (_Boost_MISSING_DEPENDENCIES)
[  196s]   CMakeLists.txt:594 (find_package)
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 25, 2017

Please see the description of http://tracker.ceph.com/issues/20048 for my analysis of the build failure.

My guess is that context is indeed not supported on s390x and, yes, this would mean that it should either be avoided or used with care so as not to break the s390x build.

@cbodley

This comment has been minimized.

Contributor

cbodley commented May 25, 2017

@tserong @smithfarm thanks, that's good to know

i do have plans to expand the use of boost::asio in radosgw outside of the beast frontend (and potentially into the Objecter), but i'll be cautious about using the stackful coroutines that depend on boost::context

@@ -512,13 +512,17 @@ endif()
option(WITH_SYSTEM_BOOST "require and build with system Boost" OFF)
set(BOOST_COMPONENTS
thread system regex random program_options date_time iostreams coroutine context)
thread system regex random program_options date_time iostreams coroutine)

This comment has been minimized.

@cbodley

cbodley May 25, 2017

Contributor

the coroutine component is also specific to the beast frontend

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 25, 2017

@cbodley Thanks for understanding. Coroutine moved; rebased.

@smithfarm smithfarm changed the title from build/ops: conditionalize rgw Beast so it isn't built on s390x architecture to build/ops: conditionalize rgw Beast frontend so it isn't built on s390x architecture May 25, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 26, 2017

@cbodley I added the debian bit. We are using this PR now downstream. Would be great to get it merged, if you agree.

@tchaikov Can you review?

@smithfarm smithfarm requested a review from tchaikov May 26, 2017

set(BOOST_HEADER_COMPONENTS container)
if(WITH_MGR)
list(APPEND BOOST_COMPONENTS python)
endif()
if(WITH_RADOSGW_BEAST_FRONTEND)

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

we should disable WITH_RADOSGW_BEAST_FRONTEND archs not listed below :

  • arm32
  • aarch64
  • i386
  • amd64
  • powerpc32
  • powerpc64
  • mips32

i am consulting asm_context_sources in boost/libs/context/build/Jamfile.v2 for the above list.

in other words, following archs are not supported:

  • alpha
  • hppa
  • ia64
  • m68k
  • mips64
  • mips64el
  • s390
  • s390x
  • sh4
  • sparc
  • sparc64
  • x32

i am consulting boost1.63-1.63.0+dfsg/debian/rules for the list above.

considering the length of these lists and their authorities, probably we should use the former.

This comment has been minimized.

@smithfarm

smithfarm May 26, 2017

Contributor

Thanks for looking that up - I can change the conditionals if you wish, though I doubt anyone will be building RPM and deb packages for mips32, for example.

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

@smithfarm debian supports mips and mipsel, so for the sake of completeness, we can list it in debian/rules like

ifneq (,$(filter $(DEB_HOST_ARCH), arm armel armhf arm64 i386 amd64 mips mipsel powerpc ppc64))
  extraopts += -DWITH_RADOSGW_BEAST_FRONTEND=ON
else
  extraopts += -DWITH_RADOSGW_BEAST_FRONTEND=OFF
endif

i understand if you intend to keep the building scripts clean and minimal. but i just wanted the packaging to be a better reference for our downstreams or potential users.

@@ -22,6 +22,10 @@ ifeq ($(DEB_HOST_ARCH), armel)
extraopts += -DWITH_ATOMIC_OPS=OFF
endif
ifeq (,$(filter $(DEB_HOST_ARCH), s390 s390x))

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

should be ifneq

This comment has been minimized.

@smithfarm

smithfarm May 26, 2017

Contributor

the conditional is supposed to be: "if DEB_HOST_ARCH equals "s390" or "s390x", then build with "-DWITH_RADOSGW_BEAST_FRONTEND=OFF"

I've never coded in this language, so I take your word for it that ifneq is correct in this case.

This comment has been minimized.

@tchaikov

tchaikov May 26, 2017

Contributor

@smithfarm see https://www.gnu.org/software/make/manual/html_node/Text-Functions.html. if it matches, a s390 or s390x is returned, which is non-empty. in that case we should pass -DWITH_RADOSGW_BEAST_FRONTEND=OFF to cmake.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 29, 2017

see also #15346, @smithfarm i can help with the debian/rules, if you'd like.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 29, 2017

see also #15347

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 29, 2017

@smithfarm could you squash related commits into a single one? and remove the merge commit?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 29, 2017

@tchaikov Yes, I'm working on that. What about "cmake: use boost target instead libary path while linking against them" and "cmake: delete excidentail reintroduced line." ? I guess we need those as well?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 29, 2017

8eb5821 has been deleted in tchaikov/ceph.git ?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 29, 2017

@tchaikov Please have another look. I guess this now obsoletes #15347, too.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 29, 2017

@smithfarm 974edd9 is more of a cosmetic change, and it fails the build in my last PR. FindBoost sometimes fails to find the built boost components. i am not sure why, though.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 29, 2017

@tchaikov OK, dropped that commit and adapted the first commit to match #15347

@tchaikov

@smithfarm thanks. Reviewed-by once shaman and OBS are happy.

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 29, 2017

@smithfarm i thought you included #15347 in this PR. but seems that's not the case. so i reopened it.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 29, 2017

@tchaikov I did, but I can't get it to build without the wholesale Boost linking.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 29, 2017

@cbodley @ktdreyer This is close to being merged; wanna take a look?

smithfarm and others added some commits May 23, 2017

cmake: build boost::context and coroutine only with rgw Beast frontend
boost::context is currently (1.63) unsupported for s390x and anyway
it makes sense to conditionalize Boost components so they are only
built with the Ceph components that need them (like is already being
done for mgr).

Fixes: http://tracker.ceph.com/issues/20048
Signed-off-by: Nathan Cutler <ncutler@suse.com>
Signed-off-by: Tim Serong <tserong@suse.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
build/ops: rpm: reverse s390 bcond conditional block
I'm not a fan of "if NOT x - then - else" blocks.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
build/ops: rpm: no rgw Beast frontend on s390x
Since the Beast frontend uses boost::context which is not supported on
s390x.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
Signed-off-by: Kefu Chai <kchai@redhat.com>
build/ops: deb: no rgw Beast frontend on s390x
Signed-off-by: Kefu Chai <kchai@redhat.com>
Signed-off-by: Nathan Cutler <ncutler@suse.com>
cmake: delete excidentail reintroduced line.
- This target got reintroduced by accident after the creation
  of ceph-common in commit:
    046b2bd

  Detection was trigger by refering to libresolv, and results in a
  linking error.

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>

@wjwithagen wjwithagen self-requested a review May 30, 2017

@wjwithagen

This overides #15340 as well.
I'll close that one.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 30, 2017

Status update: This passes Shaman, but still fails in OBS (apparently because radosgw is not explicitly linked with Boost). I'm testing a fix for that now.

cmake: link radosgw with boost libraries
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 30, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 30, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented May 30, 2017

s390x build is green. AFAICT this is ready to merge.

smithfarm added a commit to SUSE/ceph that referenced this pull request May 30, 2017

@tchaikov tchaikov merged commit 9fd1950 into ceph:master May 30, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment