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

ceph: fixes #73187

Merged
merged 2 commits into from Nov 11, 2019
Merged

ceph: fixes #73187

merged 2 commits into from Nov 11, 2019

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Nov 10, 2019

This cleans up the ceph expression a lot, and starts using the nixpkgs-provided rocksdb and gtest.

commit 06b7a9b8f3c7d1b5b51e95ea8cf075195bae3e80 (HEAD -> MMesch-nixpkgs-jupyterlab-extension-kernels, flokli/ceph-fixes)
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 23:18:48 2019 +0100

    ceph: use system-provided gtest and rocksdb

commit a0d7d1025680c79675b4974298640628e071e5de
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 23:32:56 2019 +0100

    rocksdb: enable USE_RTTI=1
    
    This is required for programs using rocksdb and and typeinfo.
    
    Otherwise, linking them fails with errors like this (that's ceph):
    
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTIN12RocksDBStore14RocksWBHandlerE[_ZTIN12RocksDBStore14RocksWBHandlerE]+0x10): undefined reference to `typeinfo for rocksdb::WriteBatch::Handler'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTIN12RocksDBStore19MergeOperatorRouterE[_ZTIN12RocksDBStore19MergeOperatorRouterE]+0x10): undefined reference to `typeinfo for rocksdb::AssociativeMergeOperator'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTIN12RocksDBStore19MergeOperatorLinkerE[_ZTIN12RocksDBStore19MergeOperatorLinkerE]+0x10): undefined reference to `typeinfo for rocksdb::AssociativeMergeOperator'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTI17CephRocksdbLogger[_ZTI17CephRocksdbLogger]+0x10): undefined reference to `typeinfo for rocksdb::Logger'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI12BlueRocksEnv[_ZTI12BlueRocksEnv]+0x10): undefined reference to `typeinfo for rocksdb::EnvWrapper'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI23BlueRocksSequentialFile[_ZTI23BlueRocksSequentialFile]+0x10): undefined reference to `typeinfo for rocksdb::SequentialFile'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI25BlueRocksRandomAccessFile[_ZTI25BlueRocksRandomAccessFile]+0x10): undefined reference to `typeinfo for rocksdb::RandomAccessFile'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI21BlueRocksWritableFile[_ZTI21BlueRocksWritableFile]+0x10): undefined reference to `typeinfo for rocksdb::WritableFile'
    /nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI17BlueRocksFileLock[_ZTI17BlueRocksFileLock]+0x10): undefined reference to `typeinfo for rocksdb::FileLock'

commit a47f3242a8044fb9e1c78fb4c74c4def3c006a54
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 23:20:15 2019 +0100

    ceph: patch more shebangs

commit c6ba0456743646778c448f2b05d84edd6e45923c
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 23:19:46 2019 +0100

    ceph: set doCheck = false explicitly
    
    and describe why.

commit 86db1bf7a9102a265c73957c8578d38f6dd64b4e
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 22:04:49 2019 +0100

    ceph: clean up PYTHONPATH wrapping
    
    Set `pythonPath` instead of exporting PYTHONPATH.
    
    Use `toPythonPath` to construct the PYTHONPATH where we need manual
    wrapping. There's no ceph-volume, only ceph-mgr.

commit d58b45ba36864d86e95c4bc36be1864074b6a6a9
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 22:03:42 2019 +0100

    ceph: fix outdated Boost::python substitutions
    
    Instead of substituting in CMakeLists.txt files, one now needs to set
    MGR_PYTHON_VERSION.

commit c4461c04a1c40e647c997b9975d9bd79d3c640ae
Author: Florian Klink <flokli@flokli.de>
Date:   Sat Nov 9 16:50:43 2019 +0100

    ceph: fix multiple output
    
    we currently just move $out/share/ceph/mgr to $lib/lib/ceph, and then
    remove all references to $out with a find command.
    
    I checked $out, the only reference to $out is in
    $lib/lib/ceph/libceph-common.so.0, coming from src/common/options.cc:
    https://github.com/ceph/ceph/blob/master/src/common/options.cc#L5050:
    
    >  Option("mgr_module_path", Option::TYPE_STR, Option::LEVEL_ADVANCED)
    >  .set_default(CEPH_DATADIR "/mgr")
    >  .add_service("mgr")
    >  .set_description("Filesystem path to manager modules."),
    
    Just removing the reference might break some behaviour - it should point
    to $lib/ceph/mgr instead.
    
    We can fix this in a much more elegant fashion by just passing a custom
    CMAKE_INSTALL_DATADIR to the build system.

commit 3d700355b4be3e0db3d37a6acea1fce69875acee
Author: Florian Klink <flokli@flokli.de>
Date:   Mon Nov 11 00:24:27 2019 +0100

    ceph: correct platforms
    
    ceph currently doesn't build on aarch64-linux. So let's not lie in
    meta.platforms.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @adevress @magenbluten for rocksdb,
cc @adevress @alexanderkjeldaas @krav @johanot for ceph

@flokli flokli mentioned this pull request Nov 10, 2019
Merged
10 tasks
jonringer
jonringer reviewed Nov 11, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

do you mind squashing the smaller commits :)?

@flokli
Copy link
Contributor Author

flokli commented Nov 11, 2019 via email

@srhb
Copy link
Contributor

srhb commented Nov 11, 2019

Tested that the dashboard is still working correctly. 👌

Note to self: Add something like this to the tests: srhb@2957120

@jonringer
Copy link
Contributor

i would probably squish them all:

git reset HEAD~8
git add pkgs
git commit -m "ceph: fix build and cleanup expression"
git push --force <fork> <branch>

@flokli
Copy link
Contributor Author

flokli commented Nov 11, 2019

Well, there's also the rocksdb RTTI change. I'll squash into 2 commits then :-)

@flokli
rocksdb: enable USE_RTTI=1
781d85c 
This is required for programs using rocksdb and and typeinfo.

Otherwise, linking them fails with errors like this (that's ceph):

/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTIN12RocksDBStore14RocksWBHandlerE[_ZTIN12RocksDBStore14RocksWBHandlerE]+0x10): undefined reference to `typeinfo for rocksdb::WriteBatch::Handler'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTIN12RocksDBStore19MergeOperatorRouterE[_ZTIN12RocksDBStore19MergeOperatorRouterE]+0x10): undefined reference to `typeinfo for rocksdb::AssociativeMergeOperator'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTIN12RocksDBStore19MergeOperatorLinkerE[_ZTIN12RocksDBStore19MergeOperatorLinkerE]+0x10): undefined reference to `typeinfo for rocksdb::AssociativeMergeOperator'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(RocksDBStore.cc.o):(.data.rel.ro._ZTI17CephRocksdbLogger[_ZTI17CephRocksdbLogger]+0x10): undefined reference to `typeinfo for rocksdb::Logger'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI12BlueRocksEnv[_ZTI12BlueRocksEnv]+0x10): undefined reference to `typeinfo for rocksdb::EnvWrapper'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI23BlueRocksSequentialFile[_ZTI23BlueRocksSequentialFile]+0x10): undefined reference to `typeinfo for rocksdb::SequentialFile'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI25BlueRocksRandomAccessFile[_ZTI25BlueRocksRandomAccessFile]+0x10): undefined reference to `typeinfo for rocksdb::RandomAccessFile'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI21BlueRocksWritableFile[_ZTI21BlueRocksWritableFile]+0x10): undefined reference to `typeinfo for rocksdb::WritableFile'
/nix/store/cg0k49h66nkdqx6ccwnqr0i4q0fnfznc-binutils-2.31.1/bin/ld: ../../lib/libos.a(BlueRocksEnv.cc.o):(.data.rel.ro._ZTI17BlueRocksFileLock[_ZTI17BlueRocksFileLock]+0x10): undefined reference to `typeinfo for rocksdb::FileLock'
@flokli
ceph: fix build and cleanup expression
05590b3 
correct platforms. ceph currently doesn't build on aarch64-linux. So
let's not lie in meta.platforms.

ceph: fix multiple output
We currently just move $out/share/ceph/mgr to
$lib/lib/ceph, and then remove all references to $out with a find
command.

I checked $out, the only reference to $out is in
$lib/lib/ceph/libceph-common.so.0, coming from src/common/options.cc:
https://github.com/ceph/ceph/blob/master/src/common/options.cc#L5050:

>  Option("mgr_module_path", Option::TYPE_STR, Option::LEVEL_ADVANCED)
>  .set_default(CEPH_DATADIR "/mgr")
>  .add_service("mgr")
>  .set_description("Filesystem path to manager modules."),

Just removing the reference might break some behaviour - it should point
to $lib/ceph/mgr instead.

We can fix this in a much more elegant fashion by just passing a custom
CMAKE_INSTALL_DATADIR to the build system.

ceph: fix outdated Boost::python substitutions

Instead of substituting in CMakeLists.txt files, one now needs to set
MGR_PYTHON_VERSION.

ceph: clean up PYTHONPATH wrapping

Set `pythonPath` instead of exporting PYTHONPATH.

Use `toPythonPath` to construct the PYTHONPATH where we need manual
wrapping. There's no ceph-volume, only ceph-mgr.

ceph: set doCheck = false explicitly

and describe why.

ceph: patch more shebangs

ceph: use system-provided gtest and rocksdb
@flokli flokli merged commit 1794b43 into NixOS:master Nov 11, 2019
@flokli flokli deleted the ceph-fixes branch November 11, 2019 20:48
@johanot
Copy link
Contributor

johanot commented Jan 20, 2020

There's no ceph-volume, only ceph-mgr.

Hmm.. @flokli help me out. what am I missing here? Looks like 05590b3 cannibalized the ceph-volume and ceph-volume-systemd binaries? was that intentional?

@srhb srhb mentioned this pull request Jan 22, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer left review comments

@krav krav Awaiting requested review from krav

@johanot johanot Awaiting requested review from johanot

@magenbluten magenbluten Awaiting requested review from magenbluten

@adevress adevress Awaiting requested review from adevress

Assignees
No one assigned
Labels
8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flokli @srhb @jonringer @johanot