-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
pywb: Init at 2.7.4 #189950
base: master
Are you sure you want to change the base?
pywb: Init at 2.7.4 #189950
Conversation
db52433
to
114916c
Compare
7e4c882
to
1e4dd94
Compare
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.
Impressive work! This truly must have been a lot of effort.
The nixpkgs way is generally to relax the dependency requirement, then only override versions if really needed.
932bc9a
to
37a4f40
Compare
pkgs/tools/misc/pywb/pywb.patch
Outdated
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.
Could you please submit this patch upstream, and then fetch it with fetchpatch
?
We'd also love to have an explanation of what it does and why it is needed. Something to the tune of "support updated jinja" i assume
pkgs/top-level/all-packages.nix
Outdated
@@ -12054,6 +12054,8 @@ with pkgs; | |||
|
|||
pywal = with python3Packages; toPythonApplication pywal; | |||
|
|||
pywb = callPackage ../tools/misc/pywb {}; |
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.
pywb = callPackage ../tools/misc/pywb {}; | |
pywb = python3.pkgs.callPackage ../tools/misc/pywb {}; |
Please also avoid using with python3.pkgs;
inside the derivation as it is considered an anti-pattern. Use the function arguments at the top of the derivation file instead. This makes the derivation more overridable
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.
bump
--replace "gevent==21.12.0" "gevent" | ||
''; | ||
|
||
propagatedBuildInputs = with python3.pkgs; [ |
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.
These don't seem to match upstream. pytest
for example should usually not be propagated. Many maintainers in nixpkgs prefer to sort the propagated inputs the same way as upstream, making it easier to spot differences. If any packages have been added that were not referenced in upstream then
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.
Extras is commonly listed in passthru.optional-dependencies
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.
bump, please don't propagate pytest
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.
builds and runs fine, but some of my prior comments has not been addressed nor argued
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.
Could you please add a pythonImportsCheck
?
--replace "gevent==21.12.0" "gevent" | ||
''; | ||
|
||
propagatedBuildInputs = with python3.pkgs; [ |
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.
bump, please don't propagate pytest
fetchSubmodules = true; | ||
}; | ||
|
||
patches = [ ./pywb.patch ]; |
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.
See comment. Please comment why this is needed and why it is not submitted upstream
pkgs/top-level/all-packages.nix
Outdated
@@ -12054,6 +12054,8 @@ with pkgs; | |||
|
|||
pywal = with python3Packages; toPythonApplication pywal; | |||
|
|||
pywb = callPackage ../tools/misc/pywb {}; |
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.
bump
seems like my "bump" comments don't show context unless you're in the files view |
Sorry about that - I think I finished the review round when I tried to merge the suggestions via the UI. I'll unmark this as a draft once it's ready for review. |
Co-authored-by: Peder Bergebakken Sundt <pbsds@hotmail.com>
Description of changes
pywb described as a "full-featured, advanced web archiving capture and replay framework for python". More info can be found at the webrecorder homepage.
This is partially based on #52814, which was never merged. I've verified on x86_64 Linux that web archive playback and recording works with the following commands:
and navigating to the UI at localhost:8080. I did not test any of the redis-based features.
Things done
sandbox = true
set 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.sh
to update generated release notes