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

foundationdb: add fdb 6.1 with cmake build #61009

Merged
merged 6 commits into from May 15, 2019

Conversation

thoughtpolice
Copy link
Member

@thoughtpolice thoughtpolice commented May 5, 2019

Motivation for this change

FoundationDB 6.1 is going to ship somewhat soon(-ish) and has a new CMake build system. While it would have been possible to keep wobbling on with the old one (a strange beast), CMake has a number of advantages and the overall result for the 6.1+ builds is looking much cleaner in the future with minimal patching, no old boost, or GCC 4.9, etc.

Some notes:

  • Allow usage of clang and libcxx as the stdenv if useClang = true. This significantly shrinks (~45%) the resulting closure size because all foundationdb binaries are statically linked to their source code (effectively duplicating large amounts of the codebase in each binary). Enabled by default, tentatively.
    • LTO doesn't quite work right in some configurations, but should hopefully offer even further significant increases (~75%!) if we can figure it out.
    • LTO + LLD in libcxxStdenv needs investigation, it dramatically seems to reduce memory requirements but something weird happens and libtls doesn't get its RPATH appropriately set(????)
  • We only need one patch this time, which will hopefully be resolved upstream eventually. For us, this patch is completely positive and only fixes a build failure with libcxx. See the discussion in flow: fix a build failure with Clang/libcxx on Linux apple/foundationdb#1533 for a path forwards.
  • The new build has a wonky layout in the install phase due to its strange fdb_install custom command, so unfortunately we still need a mostly custom install phase. I want to try and fix this so normal ninja install or whatever works (maybe I'm missing something) but it'll take more coordination upstream.
  • Tentatively using gcc8Stdenv for now as well, since it gives some binary size improvements. This can probably go away if we move to GCC 9.1 or something in stdenv (soon, hopefully!)
  • We now ship the tests that are run in the .out output. These are tiny compared to the binaries so it's not a big deal, but the improvement is that we can now run self-tests against any closures we build. (My intention is to eventually automate this and use it in some of the FoundationDB NixOS tests.)
    • Unfortunately, there's no way to get the enabled test list from CMake easily. :( I'd like to fix this upstream so we don't need test-lists.txt.

This is a draft and a WIP, but the core shape looks OK I guess. This expression is also very viable and suitable for development on FDB itself using nix-shell '<nixpkgs>' -A foundationdb61 --pure, etc.

Still TODO
  • Re-distribute the fdb-java.jar file again (should be easy, it gets built anyway).
  • Re-implement .pythonsrc so the Python bindings still work.
  • I don't understand the libtls weirdness going on in the build system, it should just detect it from buildInputs I think...
  • Find a way to get rid of test-lists.txt
    • This will probably take some work upstream, see some forum discussion. For now, this is alright I guess...
  • Think about getting rid of older FoundationDB versions? Bonus points: we can finally get rid of stdenv49 since nothing else needs it, and all the other old stuff! (The on-disk formats are still compatible, too!)
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
  • 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/)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@thoughtpolice
Copy link
Member Author

@GrahamcOfBorg build foundationdb61 python3Packages.foundationdb61

@thoughtpolice
Copy link
Member Author

Let's go ahead and merge this. The libtls shenanigans and old version support can be figured out a little later, plus some improvements to the foundationdb module will come soon hopefully...

FoundationDB is currently in the process of migrating to CMake, and it
will eventually be the only build system. In preparation for this, split
off the current (somewhat nasty) builder into its own file, and allow
default.nix to be more declarative -- containing only the main supported
versions.

Similarly, a cmake.nix file will be added later.

There is no functional change here (NFC), only an organizational change
(file moves, no hash changes).

Signed-off-by: Austin Seipp <aseipp@pobox.com>
Signed-off-by: Austin Seipp <aseipp@pobox.com>
This adds a new build of FoundationDB 6.1, using the new, much improved
with CMake build system with fewer patches and rough edges.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
This allows us to specify JSON trace logging, which is useful for
tooling to injest/transform logs.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
Slight oversight: this allows members of the FoundationDB group to read
logs.

Signed-off-by: Austin Seipp <aseipp@pobox.com>
Signed-off-by: Austin Seipp <aseipp@pobox.com>
@thoughtpolice thoughtpolice merged commit dba7af0 into NixOS:master May 15, 2019
@thoughtpolice thoughtpolice deleted the nixpkgs/fdb-61-fixes branch May 15, 2019 14:54
pull bot pushed a commit to avitex/nixpkgs that referenced this pull request May 16, 2019
This was a testing oversight that came from NixOS#61009 -- I forgot to test
the new traceFormat option with older server versions while I was
working on FDB 6.1.

Since trace_format is only available in 6.1+, emitting it
unconditionally caused older versions of the database fail to start,
reporting an error. We simply gate it behind a version check instead,
and assert the format is always XML on older versions. This avoids the
case where the user has an old version, changes traceFormat willingly,
and then is confused by why it didn't work.

As reported by @timothyklim in the comments on commit
c55b923. See

    NixOS@c55b923#r33566132

Signed-off-by: Austin Seipp <aseipp@pobox.com>
@timothyklim
Copy link
Contributor

@thoughtpolice Hi! Is there any chance that 6.1.8 (official release) will be released here soon? Thanks!

@thoughtpolice
Copy link
Member Author

@timothyklim 2a56ea3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants