etebase-server: use buildPythonApplication, default to withLdap = false#326398
etebase-server: use buildPythonApplication, default to withLdap = false#326398natsukium merged 2 commits intoNixOS:masterfrom
Conversation
ofborg
bot
added
8.has: package (new)
|
Result of 2 packages built:
|
felschr
left a comment
felschr
left a comment
There was a problem hiding this comment.
Looking good to me and tests pass on my machine with mentioned adjustment.
felschr
added
12.approvals: 1
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1821 |
Aleksanaa
left a comment
Aleksanaa
left a comment
There was a problem hiding this comment.
This package almost works in an ugly way.
This is in top-level/all-packages.nix
etebase-server = with python3Packages; toPythonApplication etebase-server;
This is in top-level/python-packages.nix:
etebase-server = callPackage ../servers/etebase { };
I just don't know why we have to shoot our foot like this. The better way is just putting it in by-name, and if it is still used as a module, use toPythonModule in python-packages.nix. This PR only makes it more confusing to read, because normally people think of python2 when seeing python (because top-level python is python2)
I agree with that. As it's an application it probably shouldn't be a python module as well, but I ofc don't know if there's any out of tree users of the module. The nixos module currently depends on python and pythonpath in passthru in the package, but that could of course be fixed.
Hm, I was about to write that in the context of a python-module it makes sense. But yeah, the file isn't in pkgs/development/python-module, so thats a good argument to actually turn things around. |
The It's been a while since I initially added the package & NixOS module, but I remember some issues why I couldn't get it working without the python module. The NixOS module invokes I didn't know about |
github-actions
bot
added
6.topic: python
pkgs/servers/etebase/default.nix
Outdated
There was a problem hiding this comment.
I guess this was just accidentally pushed? Or do you propose changing the default here?
There was a problem hiding this comment.
Yes, I do - see commit message. I'll update the PR description in a moment :)
There was a problem hiding this comment.
This should probably be mentioned in the release notes. And we might as well remove the python module then.
There was a problem hiding this comment.
Ah right, Thank you! I just removed the python package, wrote 2 sentences of release notes, squashed and rebased.
phaer
changed the title
ofborg
bot
removed
10.rebuild-darwin: 1
as it's not unsual to run it without ldap and python-ldap is currently broken on master for python 3.12.
github-actions
bot
added
8.has: documentation
ofborg
bot
added
the
10.rebuild-darwin: 1
wegank
removed
the
12.approvals: 1
There was a problem hiding this comment.
Thanks for your review! Do you mean we should not add this? It seems to a relevant, user-visible change to effectively drop a package?
pkgs/top-level/python-packages.nix
Outdated
There was a problem hiding this comment.
But do remember to add an entry in python-aliases.nix!
There was a problem hiding this comment.
Can you explain why? We have an agreement between both maintainers of the package, @felschr and myself, to drop the python package. Are you using it?
There was a problem hiding this comment.
We always add an entry there except for extra versions. Then we can also drop the release notes entry.
There was a problem hiding this comment.
Ah right, sorry i misunderstood the suggestion at first. Should be fixed now?
...not pythonPackage. This is an app, living in pkgs/servers, so
we just use callPackage in all-packages and drop the python module.
python3.pkgs.toPythonModule could be used if a python module was needed.
Before that change legacyPackages.x86_64-linux.python311.pkgs.etebase-server.python
would give a python 3.12 release, and was therefore broken.
etebase-server = toPythonModule (pkgs.etebase-server.override {
python3 = python;
});
would now be correct
github-actions
bot
removed
8.has: documentation
| email_validator = email-validator; # added 2022-06-22 | ||
| enhancements = throw "enhancements is unmaintained upstream and has therefore been removed"; # added 2023-10-27 | ||
| et_xmlfile = et-xmlfile; # added 2023-10-16 | ||
| etebase-server = throw "pkgs.python3.etebase-server has been removed, use pkgs.etebase-server"; # added 2024-07-16 |
There was a problem hiding this comment.
| etebase-server = throw "pkgs.python3.etebase-server has been removed, use pkgs.etebase-server"; # added 2024-07-16 | |
| etebase-server = pkgs.etebase-server; # added 2024-07-16 |
that should also just work
There was a problem hiding this comment.
You could end up with the wrong python version used, see this PRs description.
7 participants
I noticed that using python3 here referred to pkgs.python3 while python is always the one from the calling interpreter. In other words: before that change
legacyPackages.x86_64-linux.python311.pkgs.etebase-server.python would give me a python 3.12 release, now it's 3.11 as it should be.
as etebase-server is an app in pkgs/servers, not a library in pkgs/development/python-modules, we use buildPythonApplication and callPackage in all-packages.nix and convert with toPythonModule in python-packages.nix. The latter could be dropped in a future commit
In services.etebase-server, we now use pkgs.etebase-server instead of python3.pkgs.etebase-server. We still expose passthru.python and passthru.python path in order to avoid rewriting the module right now as well. This could be simplified in a later commit, the wrapper there shouldn't be needed.
withLdap defaults to false now, as pkgs.python3.pkgs.python-ldap is broken on master. (see e.g. python312Packages.python-ldap: fix build #326321) and its not a required feature in any way.
Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.