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

mealie: init at 1.2.0 #278454

Merged
merged 5 commits into from
Feb 26, 2024
Merged

mealie: init at 1.2.0 #278454

merged 5 commits into from
Feb 26, 2024

Conversation

litchipi
Copy link
Contributor

@litchipi litchipi commented Jan 3, 2024

Description of changes

Package request: #254260

Adds a new package mealie, with its NixOS module.

Work mostly inspired from the work previously made by @fleaz, and tested on a Qemu VM setup before being ported here.

Mealie is a self hosted recipe manager and meal planner with a RestAPI backend and a reactive frontend
application built with NuxtJS for a pleasant user experience for the whole family.
Easily add recipes into your database by providing the URL and Mealie will automatically import the relevant data or add a family recipe with the UI editor.

(See more in homepage)

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.

@litchipi
Copy link
Contributor Author

litchipi commented Jan 3, 2024

@ambroisie Can't add you as a reviewer, but here's the PR for mealie.
Also @fleaz worked on it and may want to take a look (as 90% of this is his code 😛 )

Tried to fit Contributing, let me know if you see anything.

Also: the package wasn't tested outside a systemd service, I don't really know yet if that would work directly from the ./result/bin

@ambroisie ambroisie self-requested a review January 3, 2024 12:59
Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

Took a first look at the module and package.

maintainers/maintainer-list.nix Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/mealie/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/mealie/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/mealie/default.nix Outdated Show resolved Hide resolved
pkgs/servers/web-apps/mealie/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@litchipi
Copy link
Contributor Author

litchipi commented Jan 4, 2024

@ambroisie Thank you for the detailed review, tried to address it the best I could.
I'll rebase / squash everything in clean commits once nobody has any more comment, and the whole thing is ready to merge

@litchipi
Copy link
Contributor Author

litchipi commented Jan 4, 2024

I keep getting the error

pkgs.mealie: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/me/mealie/package.nix { ... }` with a non-empty second argument.

when putting the package inside the by-name directory, so I moved it back to pkgs/servers/web-apps/mealie and it works now.
Is it really annoying if it stays here ? Am I missing something ?

@ambroisie
Copy link
Contributor

@litchipi if you move it from pkgs/servers/web-apps/mealie/default.nix to pkgs/by-name/me/mealie/package.nix and remove it from top-level/all-packages.nix then you shouldn't get an error.

The point of pkgs/by-name is that they are automatically callPackaged, so no need to do it explicitly.

@litchipi
Copy link
Contributor Author

litchipi commented Jan 5, 2024

@ambroisie Made all the changes, CI is green, ready for a new round of review ! 🙂

Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

It's starting to look very good, please bear with me for some additional changes :-)

nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/mealie.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/mealie/mealie-frontend.nix Outdated Show resolved Hide resolved
ExecStart = "${pkg.pythonPkg.gunicorn}/bin/gunicorn -b ${cfg.host}:${builtins.toString cfg.port} -k uvicorn.workers.UvicornWorker mealie.app:app";
StateDirectory = "mealie";
LogsDirectory = "mealie";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing which is missing is a credentialsFile option, I will refer you to my pyload PR as an example, but there should be plenty more in various modules.

Copy link
Member

Choose a reason for hiding this comment

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

This seems unresolved. Is this needed?

Copy link
Member

Choose a reason for hiding this comment

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

A credentialsFile option was added in 4ebf2b5 (#278454).

@fleaz
Copy link
Contributor

fleaz commented Jan 5, 2024

Thank you @litchipi for putting so much more work into this than I had! 🚀

And also a big thanks to @ambroisie for the detailed review! 🙏🏻

I will take the code and deploy it on one of my servers on the weekend, and try to spot any problems with it.

@Janik-Haag
Copy link
Member

maintainers entry lgtm; @ambroisie feel free to ping me for a merger once this is ready.

@litchipi
Copy link
Contributor Author

litchipi commented Jan 6, 2024

@ambroisie Thanks for the review, the coded looks so much cleaner now !
I managed to get it worked with most of the work done in the package building as you asked, and it's really cleaner on the nixos module side.
The whole thing looks neat now, and tests are passing on my side, so I squashed the whole thing into 2 clean commits, ready to merge if you're OK with it 😃

@fleaz
Copy link
Contributor

fleaz commented Jan 7, 2024

Just deployed this on my server and can confirm it's working as expected 👍🏻

  • I imported a dump from my previous Mealie installation into it
  • Tested the webscraper with some new recipes
  • Created/Edited some Users

@litchipi litchipi changed the title mealie: init at 1.1.0 mealie: init at 1.2.0 Feb 10, 2024
@litchipi
Copy link
Contributor Author

Change was trivial, updated to latest 1.2.0 version, and rebased over latest master 🙂

Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

Small nits.

pkgs/by-name/me/mealie/mealie-frontend.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/mealie/mealie-frontend.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/mealie/package.nix Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@ let

crfpp = stdenv.mkDerivation {
pname = "mealie-crfpp";
version = "master";
version = "unstable-2024-02-12";
Copy link
Member

Choose a reason for hiding this comment

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

The date should be the commit date.

Suggested change
version = "unstable-2024-02-12";
version = "unstable-2021-10-10";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes sorry, good catch

Copy link
Member

Choose a reason for hiding this comment

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

You accidentally reverted this.

@litchipi
Copy link
Contributor Author

The changes proposed upstream has been merged.
If this PR is OK for you, let's merge this with the patches for now, and when a new release is made, at the same time as bumping the version I'll remove the patches ?

Squashed fixups into clean commits, ready to merge if you think it's good

@Atemu
Copy link
Member

Atemu commented Feb 14, 2024

If this PR is OK for you, let's merge this with the patches for now, and when a new release is made, at the same time as bumping the version I'll remove the patches ?

Yup, that's the way to go.

@litchipi
Copy link
Contributor Author

@mweinelt Does this PR looks good for you ? 😃

Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Only small stuff, thanks for asking.

@@ -0,0 +1,51 @@
src: version:
{ lib, fetchYarnDeps, nodejs_18, prefetch-yarn-deps, stdenv }: stdenv.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

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

Is pinning the nodejs version necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, on the unpinned version I got Expected version "^14.18.0 || ^16.10.0 || ^17.0.0 || ^18.0.0 || ^19.0.0". Got "20.11.0"

, fetchFromGitHub
, fetchpatch
, makeWrapper
, python3
Copy link
Member

Choose a reason for hiding this comment

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

Use python3Packages instead. Apparently it works better for cross compilation. It maps to python3.pkgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not only need python3.pkgs, I also require python3.sitePackages, to do an override of packageOverrides for pydantic, and call python3.interpreter

Copy link
Member

@mweinelt mweinelt Feb 22, 2024

Choose a reason for hiding this comment

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

Then use python3Packages.python.sitePackages and python3Packages.python.interpreter.

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, fixed it

owner = "mealie-recipes";
repo = "crfpp";
rev = "c56dd9f29469c8a9f34456b8c0d6ae0476110516";
sha256 = "sha256-XNps3ZApU8m07bfPEnvip1w+3hLajdn9+L5+IpEaP0c=";
Copy link
Member

Choose a reason for hiding this comment

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

Generally, prefer hash over sha256, especially with SRI hashes.

ExecStart = "${pkg.pythonPkg.gunicorn}/bin/gunicorn -b ${cfg.host}:${builtins.toString cfg.port} -k uvicorn.workers.UvicornWorker mealie.app:app";
StateDirectory = "mealie";
LogsDirectory = "mealie";
};
Copy link
Member

Choose a reason for hiding this comment

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

This seems unresolved. Is this needed?

@@ -81,6 +81,8 @@ The pre-existing [services.ankisyncd](#opt-services.ankisyncd.enable) has been m

- [systemd-lock-handler](https://git.sr.ht/~whynothugo/systemd-lock-handler/), a bridge between logind D-Bus events and systemd targets. Available as [services.systemd-lock-handler.enable](#opt-services.systemd-lock-handler.enable).

- [Mealie](https://nightly.mealie.io/), a self-hosted recipe manager and meal planner with a RestAPI backend and a reactive frontend application built in NuxtJS for a pleasant user experience for the whole family. Available as [services.mealie](#opt-services.mealie.enable)
Copy link
Member

Choose a reason for hiding this comment

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

REST is a fixed term. So you can say "REST API" instead of "RestAPI backend".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy-pasted from the website, so I should maybe tell this to the creator as well 😅

@litchipi
Copy link
Contributor Author

@mweinelt Regarding #278454 (comment)

The StateDirectory is used for DATA_DIR env var, the logs isn't used anymore, so I removed it

@litchipi
Copy link
Contributor Author

Made the changes and created clean commits

Signed-off-by: Litchi Pi <litchi.pi@proton.me>
Signed-off-by: Litchi Pi <litchi.pi@proton.me>
Signed-off-by: Litchi Pi <litchi.pi@proton.me>
Signed-off-by: Litchi Pi <litchi.pi@proton.me>
@Atemu
Copy link
Member

Atemu commented Feb 26, 2024

I think we're now at the point where all major issues have been ironed out. Any further improvements can come in follow-up PRs.

Thank you so much for sticking with us and pulling through @litchipi. Not everyone contributes such a high quality PR as their first contribution.

@Atemu Atemu merged commit f53c775 into NixOS:master Feb 26, 2024
25 checks passed
Copy link
Contributor

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-278454-to-release-23.11 origin/release-23.11
cd .worktree/backport-278454-to-release-23.11
git switch --create backport-278454-to-release-23.11
git cherry-pick -x be1320f4285720b694ebb176b3d1c7d408de3b60 c1373dd3abaa278382a45b35b9bd42d38e963707 4ebf2b54b09589e35eccb1a565bfb124cb7d09ba ba9431edf8c436efebe139ebb7aa5182952078da aeb79caaf67e8aa73ac7b4b0a477f38b4d0cab09

@Atemu Atemu added 9.needs: port to stable A PR needs a backport to the stable release. and removed backport release-23.11 labels Feb 28, 2024
@Atemu Atemu mentioned this pull request Feb 28, 2024
13 tasks
@Atemu Atemu added 8.has: port to stable A PR already has a backport to the stable release. and removed 9.needs: port to stable A PR needs a backport to the stable release. labels Feb 28, 2024
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