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

Adding TypeDB #125350

Closed
wants to merge 24 commits into from
Closed

Adding TypeDB #125350

wants to merge 24 commits into from

Conversation

haskie-lambda
Copy link

@haskie-lambda haskie-lambda commented Jun 2, 2021

Motivation for this change

provide a standard way of installing typedb akin to the installers for other systems.

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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Found the cause of your SO issue and I've added some suggestions that would come up in a review.

pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
< JAVA_BIN=java
---
> JAVA_BIN=${openjdk}/bin/java
'';
Copy link
Member

Choose a reason for hiding this comment

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

Indentation is a bit odd an there's some trailing whitespace. nixpkgs-fmt can help you out.

Copy link
Author

Choose a reason for hiding this comment

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

done; did not know about nixpkgs-fmt, that helps a lot

pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
pkgs/servers/typedb/default.nix Outdated Show resolved Hide resolved
Comment on lines +37 to +49
typedb-properties-patch = ''
19c19
< server.data=server/data/
---
> server.data=${server-data}
22c22
< server.logs=server/logs/
---
> server.logs=${server-logs}
24c24
< server.port=1729
---
> server.port=${server-port}
Copy link
Member

Choose a reason for hiding this comment

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

While it's already much better, this is still a bit awkward. Ideally changes in configuration would not require a rebuild.
If we're lucky, we might still be able to use an environment variable instead of hardcoding it into the store path, although that's not the default property file behavior.

Somewhat relevant discussion https://stackoverflow.com/questions/2263929/regarding-application-properties-file-and-environment-variable. If it's built on apache commons, we're lucky.

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to specify properties or property files at runtime?

Copy link
Author

Choose a reason for hiding this comment

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

according to the typedb team not;
only at startup as a cli option or in the config file;

I decided to add a wrapper script again that makes it possible to change the three configuration options via environment variables.
at least until I am able to change the implementation of the property file reader in the typedb.

I am still debugging it, but at least it builds and the tests run fine now;
there are still some issues though...

Copy link
Member

Choose a reason for hiding this comment

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

only at startup as a cli option or in the config file;

The config file is not really a config file if it's packaged immutably with the software, so cli options are probably the easiest option. Do you plan to write a NixOS service as well?

Copy link
Author

Choose a reason for hiding this comment

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

didn't think about this before;
but I could actually do that, have to read up on that though ^^

Comment on lines +13 to +14
rm -r server_data
rm -r server_log
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rm -r server_data
rm -r server_log
rm -r server_data server_log

Comment on lines +7 to +8
mkdir "server_data"
mkdir "server_log"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mkdir "server_data"
mkdir "server_log"
mkdir server_data server_log

'';
homepage = "https://www.grakn.ai/";
license = licenses.gpl3Plus;
platforms = lib.attrNames systems;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = lib.attrNames systems;
platforms = attrNames systems;

Comment on lines +89 to +104
#patch before install
echo '${javaPatch}' > typedb_java.patch
echo '${typedb-wrapper-patch}' > typedb_wrapper.patch
patch ./typedb typedb_java.patch

mkdir $out
cp -r ./* $out
mkdir $out/bin

cat ${typedbWrapper} > $out/bin/typedb
echo "$out/typedb \"\$@\" --data=\$TYPEDB_SERVER_DATA --logs=\$TYPEDB_SERVER_LOGS --port=\$TYPEDB_SERVER_PORT" >> $out/bin/typedb
patch $out/bin/typedb typedb_wrapper.patch

chmod u+x $out/bin/typedb
rm typedb_java.patch
rm typedb_wrapper.patch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#patch before install
echo '${javaPatch}' > typedb_java.patch
echo '${typedb-wrapper-patch}' > typedb_wrapper.patch
patch ./typedb typedb_java.patch
mkdir $out
cp -r ./* $out
mkdir $out/bin
cat ${typedbWrapper} > $out/bin/typedb
echo "$out/typedb \"\$@\" --data=\$TYPEDB_SERVER_DATA --logs=\$TYPEDB_SERVER_LOGS --port=\$TYPEDB_SERVER_PORT" >> $out/bin/typedb
patch $out/bin/typedb typedb_wrapper.patch
chmod u+x $out/bin/typedb
rm typedb_java.patch
rm typedb_wrapper.patch
#patch before install
echo '${javaPatch}' > typedb_java.patch
echo '${typedb-wrapper-patch}' > typedb_wrapper.patch
patch ./typedb typedb_java.patch
mkdir -p $out/bin
cp -r ./* $out
cat ${typedbWrapper} > $out/bin/typedb
echo "$out/typedb \"\$@\" --data=\$TYPEDB_SERVER_DATA --logs=\$TYPEDB_SERVER_LOGS --port=\$TYPEDB_SERVER_PORT" >> $out/bin/typedb
patch $out/bin/typedb typedb_wrapper.patch
chmod u+x $out/bin/typedb
rm typedb_java.patch typedb_wrapper.patch

Comment on lines +6 to +23
systems = {
x86_64-linux = {
src = fetchurl {
url = "https://github.com/vaticle/typedb/releases/download/2.1.1/typedb-all-linux-2.1.1.tar.gz";
sha256 = "00wvkiahmrys32ckp6r59bbrzfd7jzsphniyjdj4p9bh7k339gn2";
};
dir = "typedb-all-linux-${typedbVersion}";
};
x86_64-darwin = {
src = fetchzip {
url = "https://github.com/vaticle/typedb/releases/download/2.1.1/typedb-all-mac-2.1.1.zip";
sha256 = "1njvssv9h0m13jz0ims96nfl1fjyk7lv8jdb6apcqpbkw7ykz8h8";
};
dir = "typedb-all-mac-${typedbVersion}";
};
x86_64-windows = {
src = fetchzip {
url = "https://github.com/vaticle/typedb/releases/download/2.1.1/typedb-all-windows-2.1.1.zip";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
systems = {
x86_64-linux = {
src = fetchurl {
url = "https://github.com/vaticle/typedb/releases/download/2.1.1/typedb-all-linux-2.1.1.tar.gz";
sha256 = "00wvkiahmrys32ckp6r59bbrzfd7jzsphniyjdj4p9bh7k339gn2";
};
dir = "typedb-all-linux-${typedbVersion}";
};
x86_64-darwin = {
src = fetchzip {
url = "https://github.com/vaticle/typedb/releases/download/2.1.1/typedb-all-mac-2.1.1.zip";
sha256 = "1njvssv9h0m13jz0ims96nfl1fjyk7lv8jdb6apcqpbkw7ykz8h8";
};
dir = "typedb-all-mac-${typedbVersion}";
};
x86_64-windows = {
src = fetchzip {
url = "https://github.com/vaticle/typedb/releases/download/2.1.1/typedb-all-windows-2.1.1.zip";
systems = rec {
x86_64-linux = {
src = fetchurl {
url = "https://github.com/vaticle/typedb/releases/download/${version}/typedb-all-linux-${version}.tar.gz";
sha256 = "00wvkiahmrys32ckp6r59bbrzfd7jzsphniyjdj4p9bh7k339gn2";
};
dir = "typedb-all-linux-${typedbVersion}";
};
x86_64-darwin = {
src = fetchzip {
url = "https://github.com/vaticle/typedb/releases/download/${version}/typedb-all-mac-${version}.zip";
sha256 = "1njvssv9h0m13jz0ims96nfl1fjyk7lv8jdb6apcqpbkw7ykz8h8";
};
dir = "typedb-all-mac-${typedbVersion}";
};
x86_64-windows = {
src = fetchzip {
url = "https://github.com/vaticle/typedb/releases/download/${version}/typedb-all-windows-${version}.zip";

Copy link
Member

Choose a reason for hiding this comment

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

@SuperSandro2000 No need for rec. You only introduce version references, which don't come from this attrset.

@stale
Copy link

stale bot commented Apr 25, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 25, 2022
@drupol
Copy link
Contributor

drupol commented Jun 6, 2023

Hi @haskie-lambda,

I'm cherry picking old PR in nixpkgs and try to push them forward.

I notice that many feedback in here were never addressed.
Are you planning to give it a refresh at some point?

If you don't, and it's perfectly fine, do you mind closing the PR?

Thanks in advance.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 6, 2023
@haskie-lambda
Copy link
Author

much of it is probably outdated anyway; I'm gonna close it for now;
if I make a new attempt, it wouldn't be too bad of an idea to start over anyway...

@drupol
Copy link
Contributor

drupol commented Jun 6, 2023

Thank you very much.

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

4 participants