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

kimai: init at 2.10.0 #296049

Closed
wants to merge 3 commits into from
Closed

kimai: init at 2.10.0 #296049

wants to merge 3 commits into from

Conversation

Sleepful
Copy link

@Sleepful Sleepful commented Mar 15, 2024

Adds Kimai package and module for NixOS

Kimai is a time-tracking app for freelancers and enterprises, offers many configurations. I have been using it for a while in my NixOS instance and I figured it was time I contribute it back here.

I am contributing the release I have been using:

https://github.com/kimai/kimai/releases/tag/2.10.0

The tool composer2nix was used to some extent to generate some of the nix files for the php package.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md. --> I hope so!

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 15, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Mar 15, 2024
ln -s /var/lib/kimai/var/cache ./var/cache
ln -s /var/lib/kimai/var/log ./var/log
ln -s /var/lib/kimai/var/data ./var/data
ln -s /var/lib/kimai/var/plugins ./var/plugins
Copy link
Author

Choose a reason for hiding this comment

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

yeah this isn't great, but given that PHP's runtime thingy symfony insists on adding run-time data to the installation directory.... this was the only way around it.

Copy link
Author

Choose a reason for hiding this comment

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

Works fine though! Look at systemd.services.phpfpm-kimai.preStart for the matching directory management.

Comment on lines +49 to +46
echo -e 'DATABASE_URL=${databaseUrl}\n' >> ./.env
echo -e 'MAILER_FROM=${mailerFrom}\n' >> ./.env
echo -e 'MAILER_URL=${mailerUrl}\n' >> ./.env
echo -e 'APP_SECRET=${appSecret}\n' >> ./.env
echo -e 'CORS_ALLOW_ORIGIN=${corsAllowOrigin}\n' >> ./.env
Copy link
Author

Choose a reason for hiding this comment

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

I tried passing these as env vars to the services.phpfpm.pools.${kimaiName} systemd service, but PHP just refused to parse the values without failing.

Copy link
Author

Choose a reason for hiding this comment

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

These work fine in the .env file though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a less than ideal way to configure a package, overriding shouldn't be necessary as it'll cause a rebuild.

Instead maybe look at making the module create a directory that mounts the sources and then generates a .env file.

@dotlambda dotlambda changed the title kimai: init at version 2.10.0 kimai: init at 2.10.0 Mar 15, 2024
@Sleepful Sleepful force-pushed the add-pkg-kimai branch 5 times, most recently from adb23a1 to 8cc6120 Compare March 15, 2024 03:39
@@ -0,0 +1,218 @@
{ config, pkgs, lib, ... }:

with lib;
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
with lib;

Copy link
Author

Choose a reason for hiding this comment

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

something wrong with lib?

Copy link
Contributor

@GGG-KILLER GGG-KILLER Mar 17, 2024

Choose a reason for hiding this comment

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

It makes difficult for anyone to immediately see what's being used from lib.

Instead use something like

let
  inherit (lib) mkOption ...;
in
  ...

Copy link
Member

Choose a reason for hiding this comment

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

@Sleepful
Copy link
Author

Sleepful commented Mar 15, 2024

ofborg-eval says:

This PR does not cleanly list package outputs after merging.

and the trace says:

error: path '/nix/store/n6h5ydm9akf58mfmafrh8x8p2908v7ih-source.drv' is not valid

What does this mean?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/ofborg-pr-error-path-is-not-valid/41507/1

@ofborg ofborg bot added 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 labels Mar 15, 2024
@Sleepful
Copy link
Author

Oh cool looks like it will pass CI :)

I will rebase the commits to clean up

@Sleepful
Copy link
Author

Also tested with my fork of nixpkgs (which took way long to compile!), the service works fine on my machine :)

@Sleepful
Copy link
Author

Final rebase to clean up commits ^

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3642

Comment on lines +33 to +36
ln -s /var/lib/kimai/var/cache ./var/cache
ln -s /var/lib/kimai/var/log ./var/log
ln -s /var/lib/kimai/var/data ./var/data
ln -s /var/lib/kimai/var/plugins ./var/plugins
Copy link
Contributor

@GGG-KILLER GGG-KILLER Mar 17, 2024

Choose a reason for hiding this comment

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

Doesn't this result in an impure package? It's altering stuff outside of its location in the nix store.

Instead this should be handled by a systemd service in a module that does this (maybe by setting the current working dir while running it or doing a bind mount of the source folders to a runtime directory).

This make it so that if people want to have 2 or more instances if kimai running on their server, they'll implicitly share the same directory as well.

Copy link
Author

@Sleepful Sleepful May 3, 2024

Choose a reason for hiding this comment

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

@GGG-KILLER It is not impure due to the creation of symlinks, the targets of the symlinks are not created, those must be created by the user on their system (or the service creates it for them).

I would like what you mention:

This make it so that if people want to have 2 or more instances if kimai running on their server, they'll implicitly share the same directory as well.

I am not sure if this is possible though. This particular PHP package insists on managing its cache (runtime) files in its installation directory.

See for example this issue: kimai/kimai#4589

It appears to be non-configurable right now.

Let me know if you think of a way in which this could be improved, but right now it seems impossible to tell Kimai to NOT use its installation directory for runtime cache. This is why the symlinks are created during installation, so that during runtime it can write outside of the store (where the symlinks point). Without the symlinks, it just fails to work properly because it attempts to write on the nix store, which it cant of course.

Copy link
Author

Choose a reason for hiding this comment

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

I agree these paths should be configurable

Comment on lines +127 to +132
users.users.${kimaiName} = {
description = "The Kimai service user";
isSystemUser = true;
group = kimaiName;
};
users.groups.${kimaiName} = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

User and group should only be created if it is the default user and group.

Comment on lines +204 to +207
mkdir -p /var/lib/kimai/var/log
mkdir -p /var/lib/kimai/var/cache
mkdir -p /var/lib/kimai/var/data
mkdir -p /var/lib/kimai/var/plugins
Copy link
Contributor

@GGG-KILLER GGG-KILLER Mar 17, 2024

Choose a reason for hiding this comment

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

These paths should be configurable so people can host multiple instances of it or use non-standard locations.

Copy link
Member

Choose a reason for hiding this comment

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

More importantly they should be using tmpfiles.d

mailerUrl ? "",
appSecret ? "",
corsAllowOrigin ? "",
...
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
...

# https://www.kimai.org/documentation/commands.html
mv ./bin/console ./bin/${consoleCmd}
'';
buildInputs = [ pkgs.cacert pkgs.curl pkgs.git ];
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
buildInputs = [ pkgs.cacert pkgs.curl pkgs.git ];
buildInputs = [ cacert curl git ];

ensureUsers = [
{
name = kimaiName;
ensurePermissions = {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't eval anymore.

Unlikely, that you need to change this one
'';
};
nginxProxy = {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the nginx vhost suboptions or omit this

};
appSecret = mkOption {
type = types.str;
default = "change_this_to_something_unique";
Copy link
Member

Choose a reason for hiding this comment

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

This should be read from a file to be secure

Copy link
Author

Choose a reason for hiding this comment

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

@SuperSandro2000
Is it considered safer to ask for a file path instead of a string? Because I have not seen that in a lot of other packages.

Comment on lines +45 to +48
Mysql package to use.
Note: changing this may require changing the `databaseUrl` option too.
'';
};
Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 17, 2024

Choose a reason for hiding this comment

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

Suggested change
Mysql package to use.
Note: changing this may require changing the `databaseUrl` option too.
'';
};
Mysql package to use.
Note: changing this may require changing the `databaseUrl` option, too.
'';
};

@SuperSandro2000
Copy link
Member

The tool composer2nix was used to some extent to generate some of the nix files for the php package.

Why not use buildComposerProject instead which involves less code gen?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@Sleepful
Copy link
Author

Sleepful commented May 3, 2024

Why not use buildComposerProject instead which involves less code gen?

@SuperSandro2000 No specific reason. I did not know of buildComposerProject at the time of this PR. I saw other projects using node2nix and it seemed like a very solid approach, instead of relying on the package versions in the nixpkgs.

Additionally it took me a good amount of work to get this working with the current configuration. Do you consider that I should re-write the PR to use buildComposerProject instead? I would rather not, but I understand the issue of diffs that could become a risk due to difficulty to review. Looking forward to your thoughts.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented May 19, 2024

I saw other projects using node2nix and it seemed like a very solid approach, instead of relying on the package versions in the nixpkgs.

But that's exactly what we want to do. We want to link against the system libraries provided by us and not some vendored things.

Also both composer2nix and node2nix rely on code generation which is not the preferred method anymore as it makes upgrades harder and more complicated and requiring bigger custom update scripts.

Do you consider that I should re-write the PR to use buildComposerProject instead?

Yeah, that would probably be the best solution.

I would rather not, but I understand the issue of diffs that could become a risk due to difficulty to review.

That shouldn't be a problem.

@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Jun 1, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 2, 2024
@sivizius
Copy link
Contributor

Obsolete by #353187

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: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (new) This PR adds a module in `nixos/` 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 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants