Bug 444366 - (deco) Review Request: deco - Extractor for various archive file formats
Review Request: deco - Extractor for various archive file formats
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
:
Depends On: deco-archive
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-27 19:16 EDT by Steve Milner
Modified: 2008-12-17 19:43 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-17 19:31:05 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ville.skytta: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Steve Milner 2008-04-27 19:16:08 EDT
Spec URL: http://www.stevemilner.org/tmp/deco.spec
SRPM URL: http://www.stevemilner.org/tmp/deco-0.8.1-1.fc8.src.rpm
Description: deco is a Un*x script able to extract various archive file formats
with features like consistent behavior, consistent interface
and much more.
Comment 1 Steve Milner 2008-04-27 19:21:39 EDT
rpmlint outputs the following ... this is due to how the application matches
file extensions with how to extract them ..

[steve@tachikoman i386]$ rpmlint deco-0.8.1-1.fc8.i386.rpm 
deco.i386: E: compressed-symlink-with-wrong-ext /usr/share/deco/tbz tar.bz2
deco.i386: E: compressed-symlink-with-wrong-ext /usr/share/deco/taz tar.gz
deco.i386: E: compressed-symlink-with-wrong-ext /usr/share/deco/tar.z tar.gz
deco.i386: E: compressed-symlink-with-wrong-ext /usr/share/deco/tgz tar.gz
[steve@tachikoman i386]$ 
Comment 2 manuel wolfshant 2008-04-28 06:34:41 EDT
Just a note after a quick glance over the spec:
- "Source0:" should include the full path to the downloadable source file, i.e.
 http://hartlich.com/deco/download/deco-0.8.1.tar.gz
- you should not use absolute paths in the spec, but macros. hence all
occurrences of "/usr" should be replaced by "%{_prefix}", "/usr/bin/" by
"%_bindir" and "/usr/share/" by "%{_datadir}"
- /usr/share/deco is not owned by anyone

 And just as a mater of curiosity: wouldn't it be simpler to use
"%{_datadir}/deco/*" (or even "%{_datadir}/%{name}") in %files, instead of
listing all individual components ?
Comment 3 Steve Milner 2008-04-28 10:34:07 EDT
I've uploaded an updates spec/srpm fixing the issues found  ...

Spec URL: http://www.stevemilner.org/tmp/deco.spec
SRPM URL: http://www.stevemilner.org/tmp/deco-0.8.1-2.fc8.src.rpm
Comment 4 Patrice Dumas 2008-04-28 16:14:14 EDT
The crename and symlinktarget C programs do the same than rename 
(util-linux-ng) and readlink (coreutils). 

So I'd suggest using those directly.

Another issue is that %{_datadir} is not used, instead %{_prefix}/share
is used. There are many ways to fix that. The cleanest would certainly 
be to add a makefile variable for PREFIX/share and substitute is in 
deco.in, especially if you can submit such patch upstream.

Comment 5 Steve Milner 2008-04-29 16:23:31 EDT
Thanks Patrice,

Are these items that need to be worked on (or patches accepted) upstream before
this package can be approved?
Comment 6 Patrice Dumas 2008-04-29 18:30:49 EDT
These issues have to be worked on, but not necessarily upstream.
It is better if you submit patches upstream at the same time you 
use them in the package, but it is not an obligation.

Using directly rename and readlink need not to be done upstream
since they may be problematic for portability. I would simply 
do a sed substitution on deco.
Comment 7 Ville Skyttä 2008-04-30 01:39:47 EDT
I think it's a bad idea to have Requires: on extractors of all supported file
types.  The current list of "flac, arj, arc, cabextract, lzma" already looks
hopelessly incomplete and at the same time personally I'd already be annoyed if
installing deco would pull in things like arj or arc.  So FWIW, +1 to not
depending on *any* archivers.

On the other hand, if the supported archive types can be enabled on the fly by
placing symlinks somewhere (that's what I understood from the discussion,
haven't looked at the package), perhaps consider using %triggers (still without
dependencies!), see eg. the bash-completion package.
Comment 8 Patrice Dumas 2008-05-01 22:59:33 EDT
(In reply to comment #7)
> I think it's a bad idea to have Requires: on extractors of all supported file
> types.  The current list of "flac, arj, arc, cabextract, lzma" already looks
> hopelessly incomplete and at the same time personally I'd already be annoyed if
> installing deco would pull in things like arj or arc.  So FWIW, +1 to not
> depending on *any* archivers.

Agreed.

> On the other hand, if the supported archive types can be enabled on the fly by
> placing symlinks somewhere (that's what I understood from the discussion,
> haven't looked at the package), perhaps consider using %triggers (still without
> dependencies!), see eg. the bash-completion package.

This looks complicated to me. It seems to me that patching deco to have
a way to tell that the program is not there and have a nice error message
in that case would be better.
Comment 9 Ville Skyttä 2008-06-25 13:28:51 EDT
Any progress here?
Comment 10 Steve Milner 2008-06-25 13:47:02 EDT
Sorry, no I have not made any progress on this.
Comment 11 Debarshi Ray 2008-09-26 01:51:29 EDT
Ping?
Comment 12 Patrice Dumas 2008-10-12 06:13:05 EDT
Upstream changed a lot, now it is a C program and there is a separate
deco-archive.
Comment 13 Steve Milner 2008-10-13 09:29:51 EDT
Sorry folks but I'm going to bow out on this :-/ ... anyone else is free to pick it up and take it from here if they have the time.
Comment 14 Orcan Ogetbil 2008-10-28 00:38:01 EDT
I think this package now belongs to a 3rd party repo since it now supports non-free formats such as .rar
It can be patched to support only free formats and a non-free subpackage can be made and put in a 3rd party repo but that's unnecessarily more work.
I can take the package over at rpmfusion now if you close the bug here. Thanks.
Comment 15 Orcan Ogetbil 2008-10-28 01:04:47 EDT
Nevermind, even bash-completion has references to non-free packages such as unrar. Since that package is allowed in Fedora, this one can be too. I will take the package over here.
Comment 16 Steve Milner 2008-10-28 15:37:11 EDT
Thanks Orcan!
Comment 17 Orcan Ogetbil 2008-10-28 15:58:02 EDT
No problem Steve, we can still maintain it together if you want.
Here are the new files:
SPEC: http://oget.fedorapeople.org/review/deco.spec
SRPM: http://oget.fedorapeople.org/review/deco-1.5.6-1.fc9.src.rpm

There are three types of rpmlint errors/warnings, which can all be ignored:

   * deco.x86_64: W: dangerous-command-in-%trigger
   I used the %trigger functions as Ville suggested. This results in these 
   "dangerous commands".

   * deco.x86_64: E: zero-length 
   All zero length files are needed by the program

   * deco.x86_64: E: compressed-symlink-with-wrong-ext
   These kinds of files are needed by the program too.

The deco program itself doesn't mean much without the deco-archive. Hence in this package I included both of them.

Please let me know if you have any suggestions for improvement.
Comment 18 Orcan Ogetbil 2008-10-28 22:53:49 EDT
Package is in good condition. My notes:

* Source0 must be given in full URL.

* I would include the test.py and demo.py inside the %doc files, because they give a nice demonstration about how gnuplot-py should be used. You don't need to remove shebangs from them. Just do:
   mv %{buildroot}%{python_sitelib}/Gnuplot/demo.py doc/Gnuplot/
   mv %{buildroot}%{python_sitelib}/Gnuplot/test.py doc/Gnuplot/
in the %install section. Or maybe you may want to install them inside an "examples" directory under %doc.

Otherwise the package is good to go.
Comment 19 Orcan Ogetbil 2008-10-28 23:02:54 EDT
Oops, sorry, comment #18 belongs to some other bug. Please ignore.
Comment 20 Orcan Ogetbil 2008-10-29 23:50:37 EDT
I separated deco-archive from deco (Bug# 469134). Therefore one of them will not need to be rebuilt everytime the other one is updated by the upstream. 

Right now, there is a circular dependency between the two packages. I'm not sure if this is allowed. But obviously they both need each other. Any ideas are welcome.

Also, because of the separation, the rpmlint output for deco is silent now. Those messages made a trip to deco-archive.

The new files are:
SPEC: http://oget.fedorapeople.org/review/deco.spec
SRPM: http://oget.fedorapeople.org/review/deco-1.5.6-2.fc9.src.rpm
Comment 21 Orcan Ogetbil 2008-11-19 18:36:42 EST
SPEC: http://oget.fedorapeople.org/review/deco.spec
SRPM: http://oget.fedorapeople.org/review/deco-1.5.7-1.fc9.src.rpm

%changelog
- Version update
Comment 22 Ville Skyttä 2008-11-20 15:48:23 EST
License should be GPLv3 instead of GPL+.

Having the scripts in /usr/share/deco and the trigger symlink logic in deco-archive will result in failures with read-only /usr/share (%{_netsharedpath}).  The dir where deco looks for the scripts (ie. where they are symlinked in) would be better placed in /var/lib/deco.  The "source" script dir is fine in /usr/share/deco.
Comment 23 Orcan Ogetbil 2008-11-20 16:20:28 EST
Hi Ville,
I wasn't very sure what to put for the license tag since no source file that contains code specifies a license in its header. There is only the LICENSE file that claims GPL3, even that file just mentions it in two lines and does not give the full text of GPL3.

I have seen in some reviews that whenever the source code files do not specify a specific version of GPL, a generic version (like GPL+ ot GPLv2+) was picked.

So do you think that whatever says in that LICENSE file, is enough to specify that the package is GPL3?

I'm doing the other changes you asked and update the packages soon. Thanks for the review.
Comment 24 Ville Skyttä 2008-11-20 17:25:54 EST
I'm confident that GPLv3 would be the correct choice for this package.  GPL+ is correct when a package just includes the general GPL text (usually as COPYING) without being more specific anywhere else (because that's exactly what the GPL text says - any version of the GPL goes then).  GPLv2+ on the other hand should be chosen only when something in the package actually explicitly says it is "v2 or later".  The only mention of the license here is in the LICENSE file, and it is explicit about GPL v3.  It would not hurt to ping upstream to include the whole GPL v3 text in the package and license notices in source files though, but that's not a requirement for the package to pass review.
Comment 25 Orcan Ogetbil 2008-11-20 22:25:44 EST
Alright, files updated:

SPEC: http://oget.fedorapeople.org/review/deco.spec
SRPM: http://oget.fedorapeople.org/review/deco-1.5.7-2.fc9.src.rpm

%changelog
- License is GPLv3.
- The extraction scripts will be inside %%{_var}/lib/%%{name}.


note: I updated deco-archive too, accordingly.
Comment 26 Ville Skyttä 2008-11-30 13:49:39 EST
Approved.
Comment 27 Orcan Ogetbil 2008-12-01 17:38:18 EST
New Package CVS Request
=======================
Package Name: deco
Short Description: Extractor for various archive file formats
Owners: oget
Branches: F-9 F-10
InitialCC:
Comment 28 Kevin Fenzi 2008-12-03 20:04:22 EST
cvs done.
Comment 29 Fedora Update System 2008-12-03 23:17:47 EST
deco-1.5.7-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/deco-1.5.7-2.fc10
Comment 30 Fedora Update System 2008-12-03 23:43:13 EST
deco-1.5.7-2.fc9.1 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/deco-1.5.7-2.fc9.1
Comment 31 Fedora Update System 2008-12-06 23:18:36 EST
deco-1.5.7-2.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update deco'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-10788
Comment 32 Fedora Update System 2008-12-06 23:29:05 EST
deco-1.5.7-2.fc9.1 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update deco'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-10877
Comment 33 Fedora Update System 2008-12-17 19:31:01 EST
deco-1.5.7-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 34 Fedora Update System 2008-12-17 19:43:07 EST
deco-1.5.7-2.fc9.1 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.