Red Hat Bugzilla – Bug 508750
Review Request: trash-cli - Command line interface to the freedesktop.org trashcan
Last modified: 2013-03-13 01:45:14 EDT
Spec URL: http://sundaram.fedorapeople.org/packages/trash-cli.spec SRPM URL: http://sundaram.fedorapeople.org/packages/trash-cli-0.11.1.2-1.fc11.src.rpm Description: trash-cli provides a command line trash usable with GNOME, KDE, Xfce or any freedesktop.org compatible trash implementation. The command line interface is compatible with rm and you can use trash as an alias to rm. A previous review request submitted was stalled and closed. This is a attempt to revive it from where it was left off.
*** Bug 448122 has been marked as a duplicate of this bug. ***
It may be nice, if you can update to the most current 0.11.2 release.
The version I packaged was the only listed as the "featured download". I have dropped a mail to the primary upstream developer asking for clarification. Meanwhile, if you can do a review, I can import and update later even.
I would volunteer to do this review. I had a quick look at the package and here are some thoughts: * Did you hear anything from upstream regarding 0.11.2? * The package includes the script "volume-of" which - does not work (wrong python module imported) - has a very generic name - does not seem to be necessary - is not mentioned in the README.txt - has no manpage I think it can be omitted. ;-) * The URL of Source0 is different from the download URL mentioned on the homepage of the project: http://trash-cli.googlecode.com/files/trash-cli-0.11.1.2.tar.gz * the usage of %{__...} commands should be consistent - either use them entirely or omit them completly (the usage of %{__python} for determining python_sitelib is ok, since it is consistent with the packaging guidelines, however, I would not use %{__sed} ...) * Please add a short comment regarding the changes to the source files in the %prep section * What's the difference between trash-restore and restore-trash in the package? The latter doesn't have a man-page... * IMHO the usage of %{pyver} could be omitted, just use e.g. %{python_sitelib}/trash_cli*.egg-info
Some more comments: * URL should be http://trash-cli.googlecode.com/ * License is GPLv2+, take a look at the headers of the scripts ("...any later version"). * Try to preserve timestamps when modifying files in %prep. Can be done similar to https://fedoraproject.org/wiki/PackageMaintainers/PackagingTricks#Convert_encoding_to_UTF-8 * Timestamp of Source0 does not match * Most of the docs are missing, at least AUTHORS and COPYING *must* to be included in the package * What's the use of this? install -d $(dirname %{buildroot}%{_mandir}) mv %{buildroot}/usr/man %{buildroot}%{_mandir}
Sorry for the delay. New upstream release: * Removed a couple of scripts * Changed URL's * Changed license tag I have retained the dos2unix stuff to keep it simple. Don't think there is any benefit to preserving timestamps in this case. Timestamp should match now Changed the docs Added a comment explaining the workaround for the man page stuff http://sundaram.fedorapeople.org/packages/trash-cli.spec http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-1.fc12.src.rpm
Hi, I've started an initial review. First, overall comments: rpmlint reports this on the spec file: trash-cli.spec: W: no-buildroot-tag It's true that buildroot isn't used in *this* version of Fedora, but if this package is copied elsewhere it could lead to disaster. Could you please add a buildroot tag? It's not a requirement, but I suggest appending "/" for all items in the "%files" section that are directories. That makes it really obvious which entries are directories. According to: http://fedoraproject.org/wiki/Packaging/Python/Eggs The invocation of "... setup.py install" should use "%{__python}" instead of "python". I did an "rpmbuild -bp" and didn't find any .pyo or .pyc files (good). After building, rpmls looks okay. You could simplify the spec file a little by changing %files to just include: %{python_sitelib}/* That would collapse 2 %files entries to one, and then you wouldn't need the "pyver" macro at all. The license looks correct. The included PKG-INFO and setup.py files say that the liecnse is "GPL v2", and the included file COPYING is the text of GPL v2. I ran mock (mock --rebuild ~/rpmbuild/SRPMS/trash-cli-0.11.2-1.fc12.src.rpm), and it built without trouble. Good! The description text isn't quite right. You can't say "trash" to replace "rm", because there is no "/usr/bin/trash". Simiarly, the "trash-put" man page is just wrong; the man page says you're supposed to say "trash FILE", but this package requires that you say "trash-put FILE". I recommend that this package install a /usr/bin/trash, and that "man trash" also works. You can make /usr/bin/trash a symlink to trash-put, if you'd like. That way, the docs are right, *AND* it makes it easy for people to invoke (just replace "rm" with "trash").
Here's a formal review, using the checklist from: http://fedoraproject.org/wiki/Packaging:ReviewGuidelines Beyond my comments in comment 7, as noted below this package MUST be modified so that %install starts with: rm -rf %{buildroot} * MUST: rpmlint must be run on every package. The output should be posted in the review.[1] As noted above, the only output is this warning: SPECS/trash-cli.spec: W: no-buildroot-tag (Please fix.) * MUST: The package must be named according to the Package Naming Guidelines OK * MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] . OK * MUST: The package must meet the Packaging Guidelines . OK other than comments per comment 7. * MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . OK. GPLv2+. * MUST: The License field in the package spec file must match the actual license. [3] OK. See analysis in comment 7. * MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4] OK. Spec includes: %doc ... COPYING * MUST: The spec file must be written in American English. [5] OK * MUST: The spec file for the package MUST be legible. [6] OK * MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. OK. $ cd ~/temp $ wget http://trash-cli.googlecode.com/files/trash-cli-0.11.2.tar.gz $ md5sum trash-cli-0.11.2.tar.gz ~/rpmbuild/SOURCES/trash-cli-0.11.2.tar.gz d78cdd9fe9620042680769244b77a77a trash-cli-0.11.2.tar.gz d78cdd9fe9620042680769244b77a77a /home/rpmbuilder/rpmbuild/SOURCES/trash-cli-0.11.2.tar.gz * MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7] OK. Built without and by using mock. * MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8] NA. It's a noarch. * MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. OK. Tested with mock. * MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9] * MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10] NA. Noarch. * MUST: Packages must NOT bundle copies of system libraries.[11] OK * MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12] NA * MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13] OK * MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [14] OK * MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [15] OK * MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [16] OK * MUST: Each package must consistently use macros. [17] OK. %{buildroot} style. * MUST: The package must contain code, or permissable content. [18] OK * MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [19] NA * MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [19] OK * MUST: Header files must be in a -devel package. [20] NA * MUST: Static libraries must be in a -static package. [21] NA * MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [22] NA * MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [20] NA * MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [23] NA * MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[21] OK * MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [24] NA. * MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [25] OK * MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [26] FAIL. Need to modify "%install" to include this at beginning. * MUST: All filenames in rpm packages must be valid UTF-8. [27] OK (SHOULDs are NOT required.) * SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [28] NA. License text included as separate file. * SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [29] No. Not available from this upstream. * SHOULD: The reviewer should test that the package builds in mock. [30] I did that; it works fine. * SHOULD: The package should compile and build into binary rpms on all supported architectures. [31] It's a noarch, so this should be true. Koji will catch this anyway. * SHOULD: The reviewer should test that the package functions as described. OK. A package should not segfault instead of running, for example. I removed a file with "trash-put JUNK", and it was moved into .local/share/Trash. trash-list correctly listed it. "trash-restore /home/rpmbuilder/temp/JUNK" restored it. All okay. * SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [32] NA * SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [23] NA * SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [22] NA * SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [33] NA * SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.[34] OK. Man pages are there for all 4 programs in /usr/bin.
(In reply to comment #7) > You could simplify the spec file a little by changing %files to just include: > %{python_sitelib}/* > That would collapse 2 %files entries to one, and then you wouldn't need the > "pyver" macro at all. This is a bad idea because the build will no longer fail if the egg fails to build. The %{pyver} macro can be dropped nevertheless by simply changing this line to %{python_sitelib}/trash_cli-*.egg-info or similar. > The license looks correct. The included PKG-INFO and setup.py > files say that the liecnse is "GPL v2", and the included file COPYING > is the text of GPL v2. By only looking at the COPYING or the egg you cannot really desinguish beteween "GPLv2 only" or "GPLv2 or any later version". You need to look at the headers of the source code and they clearly say it's GPLv2+ > I recommend that this package install a /usr/bin/trash, and that "man trash" > also works. You can make /usr/bin/trash a symlink to trash-put, if you'd like. > That way, the docs are right, *AND* it makes it easy for people to invoke > (just replace "rm" with "trash"). On the one hand you are right on the other it contradicts http://fedoraproject.org/wiki/Packaging:Conflicts#Potential_Conflicting_Files and https://fedoraproject.org/wiki/Packaging_tricks#Use_of_common_namespace
Re: comment 7 - I agree, it's better to explicitly list the egg in %files so that future failures will be caught. And we both agree it's GPLv2+. I disagree about the name "trash"; I think it should be /usr/bin/trash. Yes, the guidelines recommend caution for short names, and I agree with those guidelines. But I think the idea here is that trash is to make this the *standard* name for this functionality... changing it to "trash-put" will diminish its utility as a standard name across distros. Note, for example, that Debian uses the name "trash" for the executable of this very package, and Debian has at least as many naming-conflict concerns as Fedora: http://packages.debian.org/lenny/all/trash-cli/filelist Ubuntu also calls it "trash": http://packages.ubuntu.com/jaunty/all/trash-cli/filelist If there are eventually multiple programs named "trash", we can make them alternatives and invoke the alternatives setup.
(In reply to comment #10) > Note, for example, that Debian uses the name "trash" for the executable of this > very package, and Debian has at least as many naming-conflict concerns as > Fedora: Not sure if Debian actually has a guideline for naming conflicts. Debian happily uses "Conflicts:" http://www.debian.org/doc/debian-policy/ch-relationships.html#s-conflicts while Fedora tries to avoid it whenever possible: https://fedoraproject.org/wiki/Packaging/Guidelines#Conflicts > http://packages.debian.org/lenny/all/trash-cli/filelist > Ubuntu also calls it "trash": > http://packages.ubuntu.com/jaunty/all/trash-cli/filelist If Debian uses it, Ubuntu will use it too. ;) > If there are eventually multiple programs named "trash", we can make them > alternatives and invoke the alternatives setup. We don't use alternatives that much in Fedora. I suggest to stick with trash-put, but on the other hand I wouldn't really mind trash. Should be up to the maintainer I think.
Well, in general, we try to be consistent with other distributions unless there's some specific reason to be different. E.G., we try to use consistent package names (http://fedoraproject.org/wiki/Packaging/NamingGuidelines#General_Naming), and I think the same concept applies to program names too. I suggest using "trash", as expected by the docs. But clearly there is not packaging requirement *requiring* this. So, I guess I'll agree with comment #11 and say it's "maintainer's decision", but I think the docs and command name need to be consistent. So if the command is "trash-put", and "trash" is not accepted, then the documentation needs to describe "trash-put" instead (which it currently doesn't do).
In case my previous comment wasn't clear, I see 3 scenarios for naming the cli command for removing files: 1. /usr/bin/trash and no /usr/bin/trash-put. "man trash" should work, and all docs refer to "trash" and not "trash-put" (since "trash-put" won't work). 2. /usr/bin/trash-put and no /usr/bin/trash. "man trash-put" should work, and all docs refer to "trash-put" (since "trash" won't work). 3. Both /usr/bin/trash and /usr/bin/trash-put (presumably one links to the other). "man trash" *and* "man trash-put" should work; once you start reading the docs, they can use either name (since either works). I suggest 1 or 3, as that would make trash-cli's use consistent with Debian and Ubuntu. That's at the cost of using a short name, but I think it's rational to give 'trash' a short name... it's widely useful. No matter what, the commands and their documentation should be *consistent*, and that's my main point :-).
http://sundaram.fedorapeople.org/packages/trash-cli.spec http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-1.fc12.src.rpm Buildroot definition and cleaning of buildroot is not necessary according to current guidelines http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag No recent version of RPM and Fedora setup requires either and rpmlint warnings can be ignored in this case Of note trash was renamed to trash-cli primarily based on opposition from Fedora on a short name so renaming it now again would be pointless and I will inform upstream to update the doc to maintain consistency I have updated other things as per comments and thanks for the review
Sigh - there's a small problem. This new spec's Release number doesn't match the Changelog. It says: Release: 1%{?dist} but the Changelog says: * Thu Feb 18 2010 Rahul Sundaram <sundaram@fedoraproject.org> - 0.11.2-2 (note the -2 at the end.) I doubt upstream will update their man page. It's this *Fedora* package that is renaming "trash" into "trash-put". Since the rename is a Fedora-local change, it'd be nice if the installed Fedora docs matched what's installed (at least in the Name, Synopsis, and Examples sections for trash-put). Since man pages aren't REQUIRED, I guess I can't REQUIRE that change, but I think I should note that. Note to others reading this review request: This spec depends on more-modern rpm, so this spec would need to be changed for older RPM-based systems like EPEL 5 or less: * %install does not start with "rm -rf %{buildroot}". This violates the guidelines of: http://fedoraproject.org/wiki/Packaging:ReviewGuidelines * No "BuildRoot" value set. This violates the *old* review guidelines. I view these as both okay, since the Fedora guidelines now specifically permit this: http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag I thought it'd be important to document that. So please fix the release, consider the man page change, and repost. You're almost there!!
I am a bit confused now and can't see where I am renaming anything and reported this issue upstream I already bumped up the release number to match the changelog Please verify again
I think the source RPM URL, as given by comment #14, isn't right. That URL implies a release number of 1 (but we're at 2!), and the release number given by it (once installed) is clearly 1 and not 2. I installed the source RPM from comment #14 by doing: cd ~/temp rm trash-cli* wget http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-1.fc12.src.rpm rpm -i trash-cli* Note that the filename has a "-1" release number! Running: grep 'Release:' ~/rpmbuild/SPECS/trash-cli.spec shows: Release: 1 which conflicts with the %changelog in the same .spec file. Interestingly, if I just look at the *spec* file URL from comment #14: http://sundaram.fedorapeople.org/packages/trash-cli.spec Then that *does* have "Release: 2". So, I think the file you're asking me to review is not the one you actually wanted me to review :-). Also: The %description isn't right. You *cannot* use "trash" as a replacement for "rm", you have to use "trash-put". I'm not a fan of renaming this executable, but if you're going to do that, the %description in the spec file should be correct. You're *really close*!
You are right I pasted the wrong url for the srpm http://sundaram.fedorapeople.org/packages/trash-cli.spec http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-1.fc12.src.rpm I am not trying to rename the executable and upstream have been informed to update the docs to match
Hi, I'm the upstream developer
Hello I have send you a mail with the details but in brief trash has been renamed to trash-put but docs don't match necessarily esp man page and the description
I still don't have the right .src.rpm to review. Please upload the .src.rpm you intend for me to review, and post the correct URL. Comment #18 *cannot* be correct. Please let me explain why. Comment #18 asks me to review: http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-1.fc12.src.rpm But this can't be right: 1. The spec file embedded in the .src.rpm is NOT the same as the spec file as posted in comment #18: http://sundaram.fedorapeople.org/packages/trash-cli.spec 2. The .src.rpm filename URL is *still* release -1 (notice the "-1" near the end), but we are at release -2. I tried to work around this myself by loading: http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-2.fc12.src.rpm That *did* load a file, but the spec file in this .src.rpm is *also* not the same as: http://sundaram.fedorapeople.org/packages/trash-cli.spec For example, the RPM spec file description in this .src.rpm file says "trash" replaces "rm", but the package actually packages "trash-put" and *NOT* "trash". You've posted a .spec file: http://sundaram.fedorapeople.org/packages/trash-cli.spec But neither .src.rpm that I have includes this spec file; they both include something *different*, and in both cases they can't be right. Please do an "rpmbuild -bs trash.spec", post the resulting file, and post the resulting .src.rpm file URL. Thanks.
Note: If I install: http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-2.fc12.src.rpm and I then replace its spec file with the spec file in: http://sundaram.fedorapeople.org/packages/trash-cli.spec then it seems to work okay, and on a quick glance this would probably pass muster. But notice that can't just use a .src.rpm file that I know about so far.
I have deleted and reuploaded everything now http://sundaram.fedorapeople.org/packages/trash-cli.spec http://sundaram.fedorapeople.org/packages/trash-cli-0.11.2-2.fc12.src.rpm
Hooray! I downloaded the files per comment #23, and everything works fine now. The only rpmlint warnings are ones that are overtaken by events (the new guidelines render them unnecessary). The RPM spec's %description correctly notes that "trash-put" (not "trash") can be used instead of "rm", and "man trash-put" works too. The man page contents aren't quite right (they still say "trash" and not "trash-put"), but that's an upstream matter. Andrea Francia: Could you fix that in a future version? Anyway, thanks for packaging this! APPROVED.
New Package CVS Request ======================= Package Name: trash-cli Short Description: Command line interface to the freedesktop.org trashcan Owners: sundaram Branches: F-12 InitialCC:
New Package CVS Request ======================= Package Name: trash-cli Short Description: Command line interface to the freedesktop.org trashcan Owners: sundaram Branches: F-12 F-13 InitialCC:
CVS done (by process-cvs-requests.py).