-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
feeds: init at 0.16.1 #92339
feeds: init at 0.16.1 #92339
Conversation
, python3 | ||
}: | ||
let | ||
listparser = python3.pkgs.buildPythonPackage rec { |
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.
Why not add this to python-packages?
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.
Nothing else depends on it, and the package also hasn't been updated in a good while (last commit on the upstream repo is from September 2017). I was under (perhaps wrong) impression Python maintainers are reluctant to accept a new package like this and making it a private / vendored package was preferred.
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.
That makes sense. I think that's a pretty good reason for keeping it private.
six | ||
]; | ||
|
||
doCheck = false; |
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 add a comment why this is disabled?
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.
Default checkPhase
doesn't seem to do anything (Ran 0 tests..
). I'm not familiar with Python ecosystem at all, perhaps I'm just missing a checkInputs
dependency but I simply don't know how to properly invoke the check phase so that tests are ran (the tests folder and its contents are present in the PyPI archive).
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.
Hmm, you might need to explicitly specify checkPhase
.
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 think this is in context of listparser
. I just added a commit that runs the tests based on what I found in tox.ini
. The test suite however fails :-(. Maybe I'm missing something obvious but based on the output I really don't understand why.. Any ideas what's happening?
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.
NVM, I was looking at the development branch of the package instead of the tagged release, test should now run and pass with the commit I just pushed.
in | ||
python3.pkgs.buildPythonApplication rec { | ||
pname = "feeds-unstable"; | ||
version = "22-06-2020"; |
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.
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.
Except for the version format, this looks good to me. Thanks @pbogdan!
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.
Thank you for packaging this!
@@ -3442,6 +3442,8 @@ in | |||
|
|||
feedreader = callPackage ../applications/networking/feedreaders/feedreader {}; | |||
|
|||
feeds = callPackage ../applications/networking/feedreaders/feeds {}; |
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've seen other packages use python3Packages.callPackage
so python packages are passed as arguments to the expression. I prefer it over accessing them through python3.pkgs
/ python3Packages
, but I'm not sure if it's the generally accepted approach.
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.
The opinions are somewhat mixed, slightly leaning to avoiding it. The main disadvantage is that it just shifts the qualified accessor to normal packages, that is, we would need to use pkgs.meson
…
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've used it before without needing to qualify normal packages:
, cmake, pytest, pytest-datadir , substituteAll, gdb
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.
There are sometimes conflicts between normal packages and python packages. You would want to use pkgconfig
or meson
from all-package.nix
but you would get the python package with the same name. (pkg-config
now has a different name but the legacy alias still conflicts.) And when https://pypi.org/project/cmake/ is packaged, your package breaks.
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.
Ah, good point 😄. I will fix that next update.
listparser = python3.pkgs.buildPythonPackage rec { | ||
pname = "listparser"; | ||
version = "0.18"; | ||
|
||
src = python3.pkgs.fetchPypi { | ||
inherit pname version; | ||
sha256 = "0hdqs1mmayw1r8yla43hgb4d9y3zqs5483vgf8j9ygczkd2wrq2b"; | ||
}; | ||
|
||
propagatedBuildInputs = with python3.pkgs; [ | ||
requests | ||
six | ||
]; | ||
|
||
doCheck = false; | ||
|
||
meta = with lib; { | ||
description = "A parser for subscription lists"; | ||
homepage = "https://github.com/kurtmckee/listparser"; | ||
license = licenses.lgpl3; | ||
maintainers = [ | ||
maintainers.pbogdan | ||
]; | ||
platforms = platforms.linux; | ||
}; | ||
}; |
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.
It might be better to move this into pkgs/development/python-modules/listparser
to separate this from Gnome Feeds, and so it can be used by other packages in the future.
I would also recommend directly pulling the sources from https://github.com/kurtmckee/listparser to run the tests.
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.
Correction: It looks like the tests are included in the pypi package, so you might not need to pull the sources directly from the git repo.
@worldofpeace You asked me to do this before for #64705. What do you suggest here?
}; | ||
in | ||
python3.pkgs.buildPythonApplication rec { | ||
pname = "feeds-unstable"; |
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.
That is the convention when obtaining a git commit instead of tag, see https://nixos.org/nixpkgs/manual/#sec-package-naming. It is also controversial: #88023 (comment)
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 just noticed that @pbogdan mentioned that the latest release version doesn't work, so I deleted my comments related to it.
}; | ||
in | ||
python3.pkgs.buildPythonApplication rec { | ||
pname = "feeds-unstable"; |
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.
That is the convention when obtaining a git commit instead of tag, see https://nixos.org/nixpkgs/manual/#sec-package-naming. It is also controversial: #88023 (comment)
b3d31c7
to
954e12d
Compare
|
|
||
src = python3.pkgs.fetchPypi { | ||
inherit pname version; | ||
sha256 = "0hdqs1mmayw1r8yla43hgb4d9y3zqs5483vgf8j9ygczkd2wrq2b"; |
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.
One issue might be that r-ryantm might not update files with multiple hashes in them.
504e3c0
to
1b52c9d
Compare
, python3 | ||
}: | ||
let | ||
listparser = import ./listparser.nix { |
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 think now it's impossible to override it in case it fails to build.. You should also be able to use callPackage
instead which is a bit more versatile - you won't need inherit lib python3;
. If we would like to not add it to python-packages.nix
, we can put in the arguments list:
, listparser ? callPackage ./listparser.nix { }
And then inside the feeds expression, use passthru = { inherit listparser; };
- this will expose it and allow it to be overridden in case it fails to build.
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.
Can you explain why it should go into passthru
? I don't quite see how that's going to help with overriding.
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.
With the passthru, one should be able to:
self: super: {
feeds-with-different-listparser = super.feeds.override {
# No need to use the `passthru` attribute, but it helps the usage be clear
listparser = super.feeds.passthru.listparser.overrideAttrs(oldAttrs: {
/* src = ...; */
});
};
}
Result of 1 package built:
|
Was there any discussion about the name? Arch and Guix call this package gfeeds (https://repology.org/project/gfeeds), everyone else calls it gnome-feeds (https://repology.org/project/gnome-feeds). |
Good point, I was thinking about this myself as well. Let's do it before it reaches stable. Renaming it's |
Motivation for this change
Add (g)feeds - an RSS/Atom feed reader for GNOME. Closes #91370.
Things done
I'm not familiar with Python / meson packaging so any feedback would be appreciated. The expression is largely based on the lollypop package.
Also one thing to note - I'm not sure what the package should be called gfeeds or just feeds. The executable is named gfeeds so is the upstream repo while the included desktop file names it as feeds and the package seems to be in a transition period.
I also picked a commit that was most recent version of master at the time I wrote the expression. The most recent tagged release had trouble parsing some of my feeds which was fixed on master so I used that instead try to hunt down the relevant patches to apply on top of a tagged release.
/cc @Metadark who expressed interest in the package
/cc @jtojnar @worldofpeace as resident GNOME experts
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)