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

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 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

Copy link
Contributor

jonringer left a comment

do you mind squashing the smaller commits :)?

@flokli

This comment has been minimized.

Copy link
Contributor Author

flokli commented Nov 11, 2019

@srhb

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

jonringer commented Nov 11, 2019

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

This comment has been minimized.

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 added 2 commits Nov 9, 2019
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'
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 force-pushed the flokli:ceph-fixes branch from 06b7a9b to 05590b3 Nov 11, 2019
@flokli flokli merged commit 1794b43 into NixOS:master Nov 11, 2019
1 check was pending
1 check was pending
grahamcofborg-eval Checking out master
Details
@flokli flokli deleted the flokli:ceph-fixes branch Nov 11, 2019
@johanot

This comment has been minimized.

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
5 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.