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

scylladb: init at 3.0.5 #61438

Merged
merged 5 commits into from Aug 10, 2019
Merged

scylladb: init at 3.0.5 #61438

merged 5 commits into from Aug 10, 2019

Conversation

workflow
Copy link
Contributor

@workflow workflow commented May 13, 2019

Motivation for this change

Initial build of ScyllaDB, the real-time big data database.

This is an MVP, let me know what should be cleaned up.

Warning: Long build ahead!

cc: @dingxiangfei2009 @snawaz

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)

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

  • Assured whether relevant documentation is up to date

  • Fits CONTRIBUTING.md.


pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libsystemtap/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libantlr3cpp/default.nix Outdated Show resolved Hide resolved
maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
@workflow
Copy link
Contributor Author

@infinisil Addressed all comments. Thank you very much for your review!

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/34

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

I've listed a few changes I hope you'll find helpful.

pkgs/development/libraries/libantlr3cpp/default.nix Outdated Show resolved Hide resolved
'';

meta = with stdenv.lib; {
description = "C++ runtime libraries of ANTLR v3";
Copy link
Member

Choose a reason for hiding this comment

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

Given the installPhase you have provided this sounds misleading. What is the difference between this package and antlr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! For some reason, even though the antlr package provides libraries, these are currently at version 2.7.7 which is incompatible with 3.x, and the existing antlr3 package does not include any libraries.

Maybe I'm missing something though?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see... If I had to guess and if I trusted my memory on the issue (which I definitely do not 😄) I would wonder if whenantlr3 was packaged for nixpkgs back in 2016 maybe the c++ support in antlr3 wasn't that good so it was just skipped... 🤷‍♂️

Maybe we should just amend the existing package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay good point, I'll look into that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended the existing package instead! Let me know if that looks okay, please.

pkgs/development/libraries/libantlr3cpp/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libantlr3cpp/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/libsystemtap/default.nix Outdated Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved

configurePhase = ''
patchShebangs ./configure.py
./configure.py --mode=release
Copy link
Member

Choose a reason for hiding this comment

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

How about using configureFlags instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point!

What I'm trying to do, instead of overriding the configurePhase, is this:

  configureScript = "./configure.py";
  configureFlags = [ "--mode=release" ];
  dontAddPrefix = true;

However for some reason, whenever cmake is available as a nativeBuildInput, it ignores my overrides and tries to use cmake instead.

Copy link
Contributor Author

@workflow workflow Jul 23, 2019

Choose a reason for hiding this comment

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

To be more precise, it adds a fixing cmake files... phase, which I can't seem to turn off and somehow leads to this:

configuring
fixing cmake files...
cmake flags: -GNinja -DCMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY=ON -DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_SKIP_BUILD_RPATH=ON -DBUILD_TESTING=OFF -DCMAKE_INSTALL_INCLUDEDIR=/nix/store/gccy6z2y06s0k8z
k743rar66438v0y7q-scylladb-3.0.5/include -DCMAKE_INSTALL_LIBDIR=/nix/store/gccy6z2y06s0k8zk743rar66438v0y7q-scylladb-3.0.5/lib -DCMAKE_INSTALL_NAME_DIR=/nix/store/gccy6z2y06s0k8zk743rar66438v0y7q-scylladb-3.0.5/lib -DCMAKE_POLICY_DEFAULT_CMP0025=NEW -DCMAKE_OSX_DEPLOYMENT_TARGET= -
DCMAKE_OSX_SYSROOT= -DCMAKE_FIND_FRAMEWORK=last -DCMAKE_STRIP=/nix/store/6k95q6vvnmspf3c0nm8q55sixsvmn9nc-binutils-2.31.1/bin/strip -DCMAKE_RANLIB=/nix/store/6k95q6vvnmspf3c0nm8q55sixsvmn9nc-binutils-2.31.1/bin/ranlib -DCMAKE_AR=/nix/store/6k95q6vvnmspf3c0nm8q55sixsvmn9nc-binutils-
2.31.1/bin/ar -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++  
-- The C compiler identification is GNU 8.3.0
-- The CXX compiler identification is GNU 8.3.0
-- Check for working C compiler: /nix/store/lfhwv8kc8c94ix3y7bn34zz4k1c4zgha-gcc-wrapper-8.3.0/bin/gcc
-- Check for working C compiler: /nix/store/lfhwv8kc8c94ix3y7bn34zz4k1c4zgha-gcc-wrapper-8.3.0/bin/gcc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /nix/store/lfhwv8kc8c94ix3y7bn34zz4k1c4zgha-gcc-wrapper-8.3.0/bin/g++
-- Check for working CXX compiler: /nix/store/lfhwv8kc8c94ix3y7bn34zz4k1c4zgha-gcc-wrapper-8.3.0/bin/g++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at CMakeLists.txt:9 (message):
  This CMakeLists.txt file is only valid for use in IDEs, please define
  FOR_IDE to acknowledge this.


-- Configuring incomplete, errors occurred!

pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
#enableParallelBuilding = true; problems on hydra

# Workaround to make the python wrapper not drop this package:
# pythonFull.buildEnv.override { extraLibs = [ thrift ]; }
Copy link
Member

Choose a reason for hiding this comment

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

That's not how it should be done. Apparently this provides Python bindings. Those bindings should be made available via python-packages.nix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out!

Would you be okay if I look at that in a separate PR?

All I did here with thrift here is to reactivate the existing 0.10 thrift from nixpkgs, it's the vanilla thrift 0.10 so to speak.

pkgs/servers/scylladb/default.nix Outdated Show resolved Hide resolved
@workflow
Copy link
Contributor Author

@aanderse Thanks for your wonderfully detailed review! Learned a lot, and addressed all comments I believe.

@workflow
Copy link
Contributor Author

workflow commented Jul 25, 2019

@FRidh Thanks for your review as well! Please see my comments.

What are the next steps to get this merged?

@aanderse
Copy link
Member

aanderse commented Aug 2, 2019

@workflow Great work! How about you squash this PR into 2 commits: adding yourself to list off maintainers and the package init?

@aanderse
Copy link
Member

aanderse commented Aug 2, 2019

ping @bjornfor @infinisil @FRidh for next steps on this.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Ideally a separate commit for every package. Other than that this looks good to merge to me, nice work :)

@infinisil
Copy link
Member

@GrahamcOfBorg build scylladb

@workflow
Copy link
Contributor Author

workflow commented Aug 2, 2019

Hmm CI seems to fail, but I can't find any useful output from the borgs. Will verify building it on nixos, traveling right now and won't have access to my remote builder until Tuesday.

It builds fine here on my Ubuntu box, but that doesn't really help without sandboxing.

@workflow
Copy link
Contributor Author

workflow commented Aug 9, 2019

@infinisil Done rebasing, thanks! Tried the build on a couple of machines, including NixOS, and it builds fine (as long as it doesn't run out of memory, which can happen when using a lot of cores)

@workflow
Copy link
Contributor Author

workflow commented Aug 9, 2019

@GrahamcOfBorg build scylladb

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Commits are still a bit messed up. The first one is good, but the second one includes parts that should be in others. Each commit should be self-contained, this means for new packages this should include the addition in all-packages.nix.

version = "3.5.2";
src = fetchurl {
url ="https://www.antlr3.org/download/antlr-${version}-complete.jar";
jar = fetchurl {
Copy link
Member

Choose a reason for hiding this comment

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

What's up with these antlr changes?

Copy link
Contributor Author

@workflow workflow Aug 10, 2019

Choose a reason for hiding this comment

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

This includes the antlr libraries in the package output, which are not currently present in antlr 3.5.nix

My initial approach was to add a new package libantlr3cpp that exposes the libs only, and @aanderse suggested to modify the existing package instead!

I'll separate the change out into its own commit

@workflow
Copy link
Contributor Author

Done and rebased.

@infinisil quite an attention to detail :)

@infinisil
Copy link
Member

Hehe yeah, I kind of hate to do it, but it's better for the long-term quality of nixpkgs, since people learn from it and can do better PR's from the start in the future

That said, I have another complaint: The commit ordering needs to be adjusted, because in the current ordering, thrift-0_10 is used in the second commit, when it's only introduced in the 5th one (meaning scylladb will fail to even evaluate in the second commit).

So the ordering should be something like

  • maintainer-list
  • libsystemtap, thrift and antlr
  • scylladb

@workflow
Copy link
Contributor Author

Haha, I actually noticed that as I rebased, and hoped to get away with it ;)
Not with this eagle-eye.

Thank you @infinisil, appreciated!

Reordered.

@workflow
Copy link
Contributor Author

Weird, Github still shows the old order here, but it's correctly reordered downstream: https://github.com/tenx-tech/nixpkgs/commits/scylladb-upstream

@infinisil
Copy link
Member

Yeah GitHub unfortunately has this bug (No it's not a feature, GitHub!)

I can see the order is correct with git log, thanks :)

@infinisil infinisil merged commit 67600d7 into NixOS:master Aug 10, 2019
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

5 participants