-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
mailman-web: use default Nixpkgs Django #84896
Conversation
The upstream version bound of <2.2 is overly pessimistic, and mailman-web works just fine on Nixpkgs' default Django 2.2. I've applied a patch to relax the bound, which I have also sent upstream[1] (which is why I included the patch rather than just doing another sed). [1]: https://gitlab.com/mailman/mailman-web/-/merge_requests/2
This looks good to me, but I have not tested mailman-web with django-2.2. @peti do you have a setup where you could give this a try? |
I think @benley has one. |
FWIW mine is running happily at https://spectrum-os.org/lists/archives with this PR. I did have some trouble where it failed to migrate the database to the new Django version, but I think this was due to an unrelated issue that had resulted in me having a corrupted database in the first place. It would be nice for somebody to verify that the update works for them. |
patches = [ | ||
(fetchpatch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mind adding a comment about what the intent of the patch is doing, and when the patch is likely not to be needed
patches = [ | |
(fetchpatch { | |
patches = [ | |
# does x, y, z. Should be able to be removed after >=X.Y.Z | |
(fetchpatch { |
Yes, but I haven't had time to work on that project for a bit. If I can get back into it soon I'll test this. |
@lheckemann are you able to test this? |
I have some general rework on the module in progress, this is part of it so I'm definitely for this change — though as I mentioned upstream maybe we should remove the django dependency (since it's a transitive dependency) rather than loosening the version bounds? That said, I'm fine with merging this with an explanatory comment as @jonringer suggested (and I'm already using this effectively) |
mayflower@477ebb7 I'd include this in the module PR and we could supersede this one with it? |
@lheckemann if I am correct, now that #86177 is in, this can be closed? |
Yes. |
Motivation for this change
The upstream version bound of <2.2 is overly pessimistic, and
mailman-web works just fine on Nixpkgs' default Django 2.2. I've
applied a patch to relax the bound, which I have also sent
upstream1 (which is why I included the patch rather than just doing
another sed).
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)