Bug 970393 - Review Request: foo2zjs - Driver for printers of various wire protocols
Summary: Review Request: foo2zjs - Driver for printers of various wire protocols
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: David Woodhouse
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 970392 (view as bug list)
Depends On: 807760
Blocks: 1050949
TreeView+ depends on / blocked
 
Reported: 2013-06-03 23:28 UTC by Christopher Atherton
Modified: 2015-12-06 01:23 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-12-06 01:23:07 UTC
Type: ---
Embargoed:
dwmw2: fedora-review+


Attachments (Terms of Use)

Description Christopher Atherton 2013-06-03 23:28:56 UTC
Spec URL: https://www.dropbox.com/s/25cb53xdr7n7puu/foo2zjs.spec
SRPM URL: https://www.dropbox.com/s/lkozw9gej6ll60s/foo2zjs-0.20130602-1.fc18.src.rpm

Description:
This package was originally maintained in RPM Fusion due to patent issues surrounding jbigkit as I understand it.  The foo2zjs package in RPM Fusion has aged significantly, and I needed to update it myself in order to get my newer printer working.  After submitting the update in RPM Fusion bugzilla, it turned out that patents on jbigkit have expired and it's now available in Fedora.  Hence, foo2zjs should be moved into Fedora also.

This is my first package, and I'm seeking a sponsor.  An individual on the RPM Fusion bugzilla said he would do the review.

Fedora Account System Username: cjatherton

Comment 1 Robin Lee 2013-06-04 04:57:51 UTC
*** Bug 970392 has been marked as a duplicate of this bug. ***

Comment 2 David Woodhouse 2013-06-04 09:05:07 UTC
There's a bunch of rpmlint warnings about this. Sorry, I feel like such a hypocrite when pointing this out, since you started with a package I created myself :)

The spelling-error warnings are all OK, I think, and the invalid-url is also fine given the way that upstream does (or *doesn't* do, more to the point) release management.

But the incorrect-fsf-address should probably be fixed (not sure of packaging guidelines there; at least in the files that get shipped lik foo2zjs-wrapper), and manual-page-warning usually does actually mean that the man page is being misrendered.

I think the dangerous-command-in-%post is probably fine too, as long as that's what other similar packages in Fedora are doing.

Comment 3 Christopher Atherton 2013-06-11 22:28:30 UTC
Ok.  Corrected FSF addresses in all files that rpmlint complains about.  I hope that's adequate.  Also, fixed all the man page warnings, which all appeared to be simple typos.

The spec and srpm are now available at the Dropbox links I posted above.

Comment 4 Christopher Atherton 2013-07-02 21:57:11 UTC
I have updated foo2zjs for Fedora 19 and given it more polish.

SPEC: https://www.dropbox.com/s/vzqpit9ten7ufle/foo2zjs.spec
SRPM: https://www.dropbox.com/s/c3i9i2s92bih2qb/foo2zjs-0.20130626-1.fc19.src.rpm

Comment 5 Joonas Sarajärvi 2013-08-29 16:26:40 UTC
The SRPM URL above does not seem to work anymore.

Comment 6 Christopher Atherton 2013-08-30 04:35:04 UTC
Sorry. Forgot to update URLs after updating the package again.  New links here:

SPEC => https://www.dropbox.com/s/vzqpit9ten7ufle/foo2zjs.spec
SRPM => https://www.dropbox.com/s/qv0s1n11t7faepa/foo2zjs-0.20130813-1.fc19.src.rpm

Comment 7 Zoltan Boszormenyi 2013-12-18 11:26:07 UTC
Informal review:

1. The package is already present in the rpmfusion-free repository.

A couple of years ago I asked why this is not present in Fedora and the answer was that it's because the JBig compression is patented. I used a HP LaserJet 1020 at the time which wasn't supported, only by this software. Shortly after that hplip started supporting a lot of their "winprinters" and setting up HP printers became simply "hp-setup". But hplip doesn't drive Konica-Minolta (and other) printers using the same ZjStream protocol, so the package is useful.

It seems the jbigkit library (which is also used by foo2zjs) is already included in Fedora. When I try to remove it, the dependencies lead to libtiff, then gdk-pixbuf2, then half of the installed universe, so the patent problem must have been eliminated. (IANAL)

About the RPM spec file and the patches included in the SRPM:

2. Have you tried sending the patches to the upstream maintainer?

3. Group tags are not needed, remove them.

4. These Requires lines are also not needed, RPM figures it out automatically:

Requires:       %{name} = %{version}-%{release}

5. rpmlint only spits one warning:

$ rpmlint -i -v foo2zjs.spec

foo2zjs.spec: W: invalid-url Source0: foo2zjs-20130813.tar.gz
The value should be a valid, public HTTP, HTTPS, or FTP URL.

0 packages and 1 specfiles checked; 0 errors, 1 warnings.

The website only lists the current latest foo2zjs.tar.gz and doesn't provide any revision history. No one can known whether the tar file in the SRPM actually matches any released version of foo2zjs.

6. The correct working of this driver needs extra binary data, like firmware files for the LaserJet 1005, 1020, etc. printers or ICM data files for others. They are fetched via the "./getweb" script but it's neither run (to include the downloaded extra data files) nor included in the binary RPM. In the second case, the user should also be told to run the script and add UDEV rules to download the firmware into the printer whenever it's turned on.

Comment 8 Jiri Popelka 2013-12-18 15:08:44 UTC
(In reply to David Woodhouse from comment #2)
> But the incorrect-fsf-address should probably be fixed (not sure of
> packaging guidelines there

https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address

Comment 9 Jiri Popelka 2013-12-18 15:10:45 UTC
Currently foo2hbpl refuses to install because of conflicts with foomatic-db - should be fixed with foomatic-db-4.0-41.20131218.fc21
https://koji.fedoraproject.org/koji/buildinfo?buildID=485668

Comment 10 Jiri Popelka 2013-12-18 18:37:49 UTC
(In reply to Zoltan Boszormenyi from comment #7)
> 4. These Requires lines are also not needed,
> Requires:       %{name} = %{version}-%{release}

+1. I don't see any reason why the other sub-packages should depend on foo2zjs.
I think it's been there because of the files in %doc.
%doc COPYING ChangeLog INSTALL README manual.pdf

I'd simply add COPYING and README (maybe Changelog) into each sub-package, I don't expect anybody to have more then one of them installed at the same time.
INSTALL is useless and manual.pdf is just a copy of man pages (so useless too).

> RPM figures it out automatically:

does it ? how come ?

Comment 11 Jiri Popelka 2013-12-18 18:46:16 UTC
To the spec file:

> Requires(post): /bin/rm

/usr/bin/rm

> %defattr(-,root,root,-)

Can be removed, see
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

> # Xerox Phaser 6110 already in foomatic-db package
> rm foomatic-db/printer/Xerox-Phaser_6110.xml
> rm PPD/Xerox-Phaser_6110.ppd
> # Samsung CLP-310 already included in foomatic-db package
> rm foomatic-db/printer/Samsung-CLP-310.xml
> rm PPD/Samsung-CLP-310.ppd
> # Konica Minolta 2430DL already included in foomatic-db package
> rm foomatic-db/printer/KONICA_MINOLTA-magicolor_2430_DL.xml
> rm PPD/KONICA_MINOLTA-magicolor_2430_DL.ppd

I've removed them from foomatic-db-4.0-42.20131218.fc21
https://koji.fedoraproject.org/koji/buildinfo?buildID=485725
So you can remove these lines and add
%{_datadir}/foomatic/db/source/printer/KONICA_MINOLTA-magicolor_2430_DL.xml
%{_datadir}/cups/model/KONICA_MINOLTA-magicolor_2430_DL.ppd.gz
to %files
and
%{_datadir}/cups/model/Xerox-Phaser_6110.ppd.gz
%{_datadir}/foomatic/db/source/printer/Xerox-Phaser_6110.xml
to %files -n foo2qpdl

Comment 12 Christopher Atherton 2014-01-30 15:24:42 UTC
New revision based on suggestions:
SPEC => https://www.dropbox.com/s/vzqpit9ten7ufle/foo2zjs.spec
SRPM => https://www.dropbox.com/s/d7216fijemfd5lz/foo2zjs-0.20140126-1.fc20.src.rpm

Comment 13 Jiri Popelka 2014-01-31 14:53:34 UTC
(In reply to Christopher Atherton from comment #12)
> SPEC => https://www.dropbox.com/s/vzqpit9ten7ufle/foo2zjs.spec

Looks good to me. But next time please increase the Release tag (even it's during review).

Comment 14 Itamar Reis Peixoto 2015-08-15 14:18:20 UTC
have you ever tried to upload spec + src.rpm to fedorapeople instead of dropbox ?

Can you do it for last version of your spec/src.rpm ?

Comment 15 Christopher Atherton 2015-11-12 03:16:34 UTC
I don't belong to any project group, so can't use fedorapeople.  (I may work on that...)

In the meantime , an update ...

SPEC => https://www.dropbox.com/s/tp49pw6n5ycb4gc/foo2zjs.spec?dl=0
SRPM => https://www.dropbox.com/s/i8936cco5xt85jg/foo2zjs-0.20151111-1.fc23.src.rpm?dl=0

Comment 16 Jiri Popelka 2015-11-12 12:35:22 UTC
Looks good to me !

There's however one rule that's changed since my comment #10 - license file should now be included using %license and not %doc.
http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
I'd be nice if you could change that, but I don't think it's a must, so I guess this package is approved !

Comment 17 Jiri Popelka 2015-11-12 12:40:59 UTC
Whoops, I forgot I have to be a sponsor to do official review of this.

Comment 18 David Woodhouse 2015-11-12 12:50:09 UTC
$ fedora-review -b 970393
INFO: Processing bugzilla bug: 970393
INFO: Getting .spec and .srpm Urls from : 970393
INFO:   --> SRPM url: https://www.dropbox.com/s/i8936cco5xt85jg/foo2zjs-0.20151111-1.fc23.src.rpm
INFO:   --> Spec url: https://www.dropbox.com/s/tp49pw6n5ycb4gc/foo2zjs.spec
INFO: Using review directory: /home/dwmw2/970393-foo2zjs
INFO: Downloading .spec and .srpm files
error: line 1: Unknown tag: <!DOCTYPE html><html lang="en" xmlns:fb="http://ogp.me/ns/fb#" xml:lang="en" class="media-desktop" xmlns="http://www.w3.org/1999/xhtml"><head><script nonce="ZZ/moT16e3Wj5hsbQ1GC">
ERROR: "Can't parse specfile: can't parse specfile\n" (logs in /home/dwmw2/.cache/fedora-review.log)

Comment 19 Jiri Popelka 2015-11-12 12:53:14 UTC
Download them manually and run
$ fedora-review -n foo2zjs

Comment 20 Christopher Atherton 2015-11-12 18:42:09 UTC
I've switched COPYING from %doc to %license.  SPEC/SRPM have the same location, but for convenience...

SPEC => https://www.dropbox.com/s/tp49pw6n5ycb4gc/foo2zjs.spec?dl=0
SRPM => https://www.dropbox.com/s/i8936cco5xt85jg/foo2zjs-0.20151111-1.fc23.src.rpm?dl=0

Comment 21 David Woodhouse 2015-11-13 08:42:16 UTC
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/foomatic/db/source,
     /usr/share/foomatic/db, /usr/share/foomatic/db/source/driver,
     /usr/share/cups/model, /usr/share/foomatic,
     /usr/share/foomatic/db/source/printer, /usr/share/foo2qpdl,
     /usr/share/foomatic/db/source/opt, /usr/share/cups

For some of those, perhaps we want Requires: foomatic-db? 

/usr/share/foo2qpdl seems like a simple omission.

Do we want to require cups-filesystem for the latter? 

Also, now I'm looking at the Requires:.... should it require /bin/rm not /usr/bin/rm?

[ ]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define foo2zjs_ver 20151111

That can be changed, can't it?

Finally (although not strictly a review comment) does any active user have one of the printers which need firmware loaded, and can we get that working automatically too?

Comment 22 Jiri Popelka 2015-11-13 09:15:45 UTC
(In reply to David Woodhouse from comment #21)
> /usr/share/foomatic
> /usr/share/foomatic/db
> /usr/share/foomatic/db/source

These are already part of foomatic-db-filesystem.

> /usr/share/foomatic/db/source/driver
> /usr/share/foomatic/db/source/opt
> /usr/share/foomatic/db/source/printer

We can move these from foomatic-db into foomatic-db-filesystem as well I think.

> /usr/share/cups
> /usr/share/cups/model

Yes, cups-filesystem is the one to require here.

> Also, now I'm looking at the Requires:.... should it require /bin/rm not
> /usr/bin/rm?

I had suggested to use /usr/bin/rm in comment #11 because:
$ rpm -ql coreutils | grep rm
/usr/bin/rm
 
I agree with your other notes.

Comment 23 Jiri Popelka 2015-11-13 09:53:38 UTC
(In reply to Jiri Popelka from comment #22)
> We can move these from foomatic-db into foomatic-db-filesystem as well I
> think.

Changed in rawhide
http://pkgs.fedoraproject.org/cgit/foomatic-db.git/commit/?id=b32fefc9e978728aa96644903c77465d14a8cc70

So adding
Requires: foomatic-db-filesystem cups-filesystem
to each (sub)package should fix the unowned directories (except the /usr/share/foo2qpdl which should be owned by foo2qpdl).

Comment 24 David Woodhouse 2015-11-13 15:38:28 UTC
Assuming that's all fixed, consider it reviewed then. Now I just need to sponsor Christopher... what is your FAS account name? I can't see you in the 'cla_done' group...

Comment 25 Christopher Atherton 2015-11-13 16:57:29 UTC
My account name is "cjatherton".

Comment 26 David Woodhouse 2015-11-14 07:04:58 UTC
Done. You should now be able to make the new package request as described at https://fedoraproject.org/wiki/PackageDB_admin_requests

Comment 27 Gwyn Ciesla 2015-11-15 22:55:03 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/foo2zjs

Comment 28 Fedora Update System 2015-11-23 01:20:30 UTC
foo2zjs-0.20151111-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-db15317851

Comment 29 Fedora Update System 2015-11-24 02:23:39 UTC
foo2zjs-0.20151111-1.fc23 has been pushed to the Fedora 23 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 'dnf --enablerepo=updates-testing update foo2zjs'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-db15317851

Comment 30 Fedora Update System 2015-11-26 00:25:30 UTC
foo2zjs-0.20151111-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-329c3c3c51

Comment 31 Fedora Update System 2015-11-26 16:55:20 UTC
foo2zjs-0.20151111-3.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-b6687a7b2d

Comment 32 Fedora Update System 2015-11-26 23:49:24 UTC
foo2zjs-0.20151111-3.fc23 has been pushed to the Fedora 23 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 'dnf --enablerepo=updates-testing update foo2zjs'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-b6687a7b2d

Comment 33 Fedora Update System 2015-11-26 23:50:09 UTC
foo2zjs-0.20151111-2.fc23 has been pushed to the Fedora 23 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 'dnf --enablerepo=updates-testing update foo2zjs'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-329c3c3c51

Comment 34 Fedora Update System 2015-12-06 01:22:59 UTC
foo2zjs-0.20151111-3.fc23 has been pushed to the Fedora 23 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.