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
fava: 1.10 -> 1.11 #68165
fava: 1.10 -> 1.11 #68165
Conversation
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.
LGTM
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.
uh oh
I didn't actually submit this review when I wrote it...
@@ -28,6 +27,11 @@ buildPythonApplication rec { | |||
simplejson | |||
]; | |||
|
|||
# CLI test expects fava on $PATH. Not sure why static_url fails. | |||
checkPhase = '' | |||
py.test tests -k 'not cli and not static_url' |
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.
Fava seems to serve its static content (application data) with mtime
in the query parameter. This is derived from the modification time of these files. Nix sets these times to 1970-01-01 01:00:01.000000000 +0100
to reduce entropy in the store. Hence, fava will serve static file urls with a broken cache busting parameter.
This feature should be patched to use the store hash instead of mtime for cache busting.
The cli test can be skipped if you can manually confirm that the cli starts.
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.
I confirmed that running fava from the cli works. And I figured out why the test for static_url
fails: it looks for files that are not included in the PyPI archive.
You're right it won't work with Nix, though. How about something like this (untested)?
file_path = os.path.join(app.static_folder, filename)
nix_store = "@storeDir@" # substituted with builtins.storeDir
if os.path.commonpath([file_path, nix_store]) == nix_store:
# Use the store hash for cache busting for files in the Nix store
# (hopefully all of them).
values["nixhash"] = os.path.relpath(file_path, nix_store)[:32]
Depends on #68163 to build.
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @matthiasbeyer