Skip to content

Add trilium server and module#75047

Merged
aanderse merged 9 commits intoNixOS:masterfrom
kampka:trilium-server
Dec 23, 2019
Merged

Add trilium server and module#75047
aanderse merged 9 commits intoNixOS:masterfrom
kampka:trilium-server

Conversation

@kampka
Copy link
Contributor

@kampka kampka commented Dec 5, 2019

Motivation for this change

This change adds the trilium server package and a module to run it.
It also refactors the trilium package as such so its easier to keep the metadata and version of both packages in sync.

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 @dtzWill @emmanuelrosa

@kampka kampka requested a review from infinisil as a code owner December 5, 2019 13:51
@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package labels Dec 5, 2019
@ofborg ofborg bot requested a review from dtzWill December 5, 2019 14:12
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 5, 2019
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 have only reviewed the nixos module. Further to the points mentioned below there are inconsistencies between your spelling trillium vs trilium.

@kampka kampka force-pushed the trilium-server branch 2 times, most recently from 9ee81be to 7f59d7c Compare December 6, 2019 11:37
@kampka
Copy link
Contributor Author

kampka commented Dec 6, 2019

@aanderse Thanks very much for the quick review, I appreciate it :)

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.

For some reason, presumably user error 😕, my mobile browser seems to have lost one of the comments I made on this PR. So here we go again... sorry for the noise.

@kampka kampka force-pushed the trilium-server branch 2 times, most recently from b42a4c3 to 947a723 Compare December 6, 2019 13:29
@kampka
Copy link
Contributor Author

kampka commented Dec 8, 2019

For some reason, presumably user error confused, my mobile browser seems to have lost one of the comments I made on this PR. So here we go again... sorry for the noise.

@aanderse for some reason GH has recorded this message as a change request.
Is there anything we need to do there to not have it block the merge?

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.

Mostly fairly minor things at this point.

@kampka kampka requested a review from aanderse December 8, 2019 13:37
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.

Module looks good to me! Thanks.

I haven't reviewed anything but the module so I'll wait for review from @dtzWill, @emmanuelrosa, or anyone else who cares to provide it. If no one jumps on this in the next couple days ping me and I'll take a look at the package changes.

@kampka
Copy link
Contributor Author

kampka commented Dec 8, 2019

@aanderse Thanks a lot for this comprehensive and instructive (also: timely ;) ) review !

@emmanuelrosa
Copy link
Contributor

I tested trilium-desktop and trilium-server. Both worked :)

I used a NixOS container to install trilium-server and then connected from the host:

trilium-container.nix

# Edit this configuration file to define what should be installed on
# your system.  Help is available in the configuration.nix(5) man page
# and in the NixOS manual (accessible by running ‘nixos-help’).

{ config, pkgs, lib, ... }:

{

   services.trilium-server = {
    enable = true;
    nginx = {
      enable = true;
      hostName = "trilium.example.com";
    };
  };

  networking.firewall.enable = false;
}

nixos-container create trilium --config-file trilium-container.nix --host-address 192.168.1.108 --local-address 10.233.0.100

@emmanuelrosa
Copy link
Contributor

I forgot to mention that trilium-server is not logging to journald. Instead it's logging to a file in dataDir.

@kampka
Copy link
Contributor Author

kampka commented Dec 9, 2019

I forgot to mention that trilium-server is not logging to journald. Instead it's logging to a file in dataDir.

It is actually doing both, although not quite consistently (https://github.com/zadam/trilium/blob/c4d5060a0b9f2066e661725c6d32ed2f016b8b71/src/services/log.js).
It's also not just the server, the desktop version does this too in ~/.local/share/trilium-data/log.
As there is no way to configure this, I see only the option to patch it on our side.
If anyone thinks it's worth it I'll prepare a patch for review

@kampka
Copy link
Contributor Author

kampka commented Dec 9, 2019

@emmanuelrosa I've added a patch for the server version that removes the file logger and logs to console only now.
The desktop version is a pre-build electron app, I don't see a good way to patch that without building from source.

@kampka
Copy link
Contributor Author

kampka commented Dec 19, 2019

@GrahamcOfBorg build trilium-server
@GrahamcOfBorg build trilium-desktop
@GrahamcOfBorg test trilium-server

@kampka
Copy link
Contributor Author

kampka commented Dec 19, 2019

Rebased against master and updated the package version once more.
Anyone willing to push the button? :)

@aanderse
Copy link
Member

@kampka oops, sorry for the delay. I didn't notice the comments from @emmanuelrosa.

Speaking of which... @emmanuelrosa do you mind hitting the approve button please? I took a very quick look over the package but I would like you as the listed maintainer to give it the official thumbs up.

@kampka can you please address the aarch64 failure?

@kampka
Copy link
Contributor Author

kampka commented Dec 22, 2019

@GrahamcOfBorg build trilium-desktop
@GrahamcOfBorg build trilium-server
@GrahamcOfBorg test trilium-server

@kampka
Copy link
Contributor Author

kampka commented Dec 22, 2019

@GrahamcOfBorg build trilium-desktop
@GrahamcOfBorg build trilium-server
@GrahamcOfBorg test trilium-server

@ofborg ofborg bot added the 8.has: clean-up This PR removes packages or removes other cruft label Dec 22, 2019
@kampka
Copy link
Contributor Author

kampka commented Dec 22, 2019

@GrahamcOfBorg test trilium-server

@kampka
Copy link
Contributor Author

kampka commented Dec 22, 2019

@aanderse sorry for the noise, the fact that ofBorg does an "empty" run on unsupported archs threw me of there.
Pinned the executables and tests to x86_64-linux

@emmanuelrosa
Copy link
Contributor

I don't have an approval button, but nevertheless, I approve :)

@aanderse
Copy link
Member

Pinned the executables and tests to x86_64-linux

I thought if you marked a package as x86_64-linux tests would only run for that platform. I don't think you need to specifically mention that in the list of tests... but I'm not sure off the top of my head.

I don't have an approval button, but nevertheless, I approve :)

If you review a package you should have the ability to approve, comment, or request changes. Thank you for explicitly stating, though 😄

As an aside... you are marked as a maintainer for this package yet you aren't a member of the NixOS github project 🤔 If this is not by choice we should ping the maintainer of that code because it was my understanding that everyone listed as a maintainer was supposed to be added to the project at one point. Let me know if you want this corrected.

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.

👍

Thanks @kampka! ...and sorry for the long drawn out process.

@aanderse aanderse merged commit 086d1ad into NixOS:master Dec 23, 2019
@emmanuelrosa
Copy link
Contributor

@aanderse, I was involved in developing the original package. I suppose that's why I'm listed as a maintainer. I'd be honored to be a member of the NixOS project.

@kampka
Copy link
Contributor Author

kampka commented Dec 23, 2019

@aanderse @emmanuelrosa thanks for taking the time to review this guys 👍

@kampka kampka deleted the trilium-server branch December 23, 2019 08:13
@aanderse
Copy link
Member

aanderse commented Dec 23, 2019

@kampka no problem. Thanks for the contribution 🎉
@emmanuelrosa I'm told this should be taken care of at some point today 😄

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

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up This PR removes packages or removes other cruft 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants