weblate: init at 4.13.1, add module#169797
Conversation
|
Hi @wiredhikari, thanks for your review. Could you add a bit on what exactly you checked about the code? Did you just look at it or did you also run the tests, try the installation? Answering that might help other people to determine wether this is mergeable. |
|
I went through the code and tests, seemed fine to me |
RaitoBezarius
left a comment
There was a problem hiding this comment.
Can I do something to help you move this PR forward?
There was a problem hiding this comment.
makeLibPath [ pkgs.pango pkgs.harfbuzz ]
There was a problem hiding this comment.
lib.makeLibraryPath [ pkgs.pango pkgs.harfbuzz ] produces /nix/store/a83690v9dsx2l2bafc1bqs17nsa8x4i8-pango-1.50.8/lib:/nix/store/jr685bs5jjs3rb9r297hma145wc5jbsn-harfbuzz-5.1.0/lib, but I think we need the girepository-1.0 bit to be included.
There was a problem hiding this comment.
This should be ideally done by wrapper (e.g. gobject-introspection + wrapGAppsNoGuiHook) in the package itself so that it works outside the context of the module: https://nixos.org/manual/nixpkgs/stable/#ssec-gnome-typelibs
There was a problem hiding this comment.
I'm currently trying to move the the gobject-introspection and PATH logic into the package, but I'm having a hard time properly doing that inside mkPoetryApplication and buildPythonApplication. Will try some more and report back.
But note that we can't completely remove the wrapping, as the settings.py is generated in the module. So the package itself will probably never be of much use outside the module.
There was a problem hiding this comment.
If it's a Django app, we can still rewire DJANGO_SETTINGS_MODULE to something else (I have already done it multiple times)
There was a problem hiding this comment.
I think static should be refactored in the module system and let the default be the path you're using.
There was a problem hiding this comment.
I'm not sure I understand your proposition? The / route must already be served by the uwsgi unix domain socket, so it's necessary to explicitly route only /static/.
There was a problem hiding this comment.
I meant it would be nice to have pkgs.weblate.static which references to ${pkgs.weblate}/lib/${pkgs.python3.libPrefix}/site-packages/weblate/static/ and have in the module system cfg.staticPackage which points to pkgs.weblate.static and here you would reference ${cfg.staticPackage}/favicon.ico for example.
So (1) someone can access to the static part of Weblate (here, it means to build the weblate derivation, so few benefits) (2) someone can replace the static part of Weblate with a patch or something easily.
There was a problem hiding this comment.
Now I get what you are aiming at, but I don't see the benefits yet.
Is this pattern used anywhere else in Nixpkgs you could point me to?
Is there any existing replacement for the static part of Weblate? I don't see the use case.
There was a problem hiding this comment.
A rapid grep shows at least:
pkgs/servers/misc/navidrome/default.nix
26: ui = callPackage ./ui {
pkgs/servers/gotify/default.nix
34: ui = callPackage ./ui.nix { };
I imagine those are more involved, as they are most likely Node-generated static files.
Maybe static is not a good name as it could get confused with a statically-linked binary from what I see in nixpkgs, but ui is often used.
Is there any existing replacement for the static part of Weblate? I don't see the use case.
There may be situations where you want to do pkgs.runCommand "patch-static" ''cp -r ${pkgs.weblate.static} $out && do something with $out to patch it, e.g. a robots.txt, etc. and pass it to cfg.staticPackage rather than patching the whole pkgs.weblate.
I have no strong feeling on this, it can be added later on if anyone has a real need for it, but most of the time, I see that people would appreciate having override options rather than locking them into a let expression ; in this case, overlaying the original weblate package would do the job as long as it is a reasonable patch.
There are no open issues from my side with this one, so reviewing it helps definetly! Thanks. |
There was a problem hiding this comment.
Maybe we can use sockets here as well so that we do not need user and password.
There was a problem hiding this comment.
Hm, sockets are used here?
There was a problem hiding this comment.
Yes, the unix socket is already used. I removed the PASSWORD and PORT option to not cause any confusion.
There was a problem hiding this comment.
This should be ideally done by wrapper (e.g. gobject-introspection + wrapGAppsNoGuiHook) in the package itself so that it works outside the context of the module: https://nixos.org/manual/nixpkgs/stable/#ssec-gnome-typelibs
There was a problem hiding this comment.
Actually, is this used anywhere other then tests?
There was a problem hiding this comment.
It would be nice to expose the django-admin stuff so that someone can do weblate-admin createsuperuser or whatever.
There was a problem hiding this comment.
@jtojnar Yeah you would use it whenever you have to run admin tasks, as I see no other way to put the correct settings.py into the PYTHONPATH.
There was a problem hiding this comment.
Maybe make settings.py in the package load /etc/weblate/settings.py or something?
There was a problem hiding this comment.
What would be a nice thing in the future would be to have a "package with a setting", i.e. cfg.weblate.withSettings (...) and the settings would be baked into it.
There was a problem hiding this comment.
But why would one want to run the package outside NixOS in the first place?
Upstreams intention seems to be, that you download the git repository on your server, run a few install scripts and then edit the settings.py by hand to your likings. Before you edit the file, their software isn't usable as well. I don't see why we should offer a way to use Weblate outside of a deployment framework, when upstream doesn't.
There was a problem hiding this comment.
Precisely to be able to do admin tasks when needed, without having to use weird weblate-env runner. That is also a thing going out of way of upstream but a one much less convenient to use.
There was a problem hiding this comment.
But why would one want to run the package outside NixOS in the first place? Upstreams intention seems to be, that you download the git repository on your server, run a few install scripts and then edit the settings.py by hand to your likings. Before you edit the file, their software isn't usable as well. I don't see why we should offer a way to use Weblate outside of a deployment framework, when upstream doesn't.
As said, no strong feeling on this. It helps a lot to manage multi-instances, diagnose existing setups, etc.
It can be done in a later PR if anyone has a need for it.
There was a problem hiding this comment.
Again, this should be already on PATH, in the package.
There was a problem hiding this comment.
This looks pretty invasive but not sure what else to do. According to https://www.postgresql.org/docs/current/pgtrgm.html, it might not be necessary to run it as postgres user.
There was a problem hiding this comment.
Yes, it is enough to add CREATE in ensurePermissions and run this under the weblate user.
There was a problem hiding this comment.
(Do note that it may provide access to the postgres user anyway because if someone can do arbitrary CREATE EXTENSION, they may be able to load their own ext and do bad stuff, the postgres solution is not that bad.)
There was a problem hiding this comment.
Didn't look further into it, but if the situation is like the two of you described I'd rather execute known code once with higher privileges, than to give the weblate user more privileges for potentially malicious code?
There was a problem hiding this comment.
Yes, I think letting as-is, is better.
There was a problem hiding this comment.
I never tried it, but I'd like to, yes. Could imagine upstream not to be very willing to change their build system / maintain a second one though.
There was a problem hiding this comment.
Maybe open an issue explaining the needs for lock files first?
Co-authored-by: Jan Tojnar <jtojnar@gmail.com>
This reverts commit ad4f421.
|
I just learned, that poetry2nix was dropped from Nixpkgs. |
Description of changes
Back in the Summer of Nix 2021 I created a Flake for Weblate: https://github.com/ngi-nix/weblate
Now I want to merge this into Nixpkgs in order to increase discoverability and likelihood of maintenance (even though I kept the flake uptodate since the summer and also plan to do so in the future.
Older attempt: #48726
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes