Bug 209082 - Review Request: scanbuttond - Scanner Button tools to SANE
Summary: Review Request: scanbuttond - Scanner Button tools to SANE
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jima
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-10-03 06:14 UTC by Parag AN(पराग)
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-10-17 18:53:27 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Proposed spec changes (2.39 KB, text/x-patch)
2006-10-10 15:35 UTC, Ralf Corsepius
no flags Details
let scanbuttond dlopen *.so.1 (1.29 KB, text/x-patch)
2006-10-10 15:36 UTC, Ralf Corsepius
no flags Details

Description Parag AN(पराग) 2006-10-03 06:14:55 UTC
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-2.src.rpm
Description: Modern scanners usually have several front panel buttons which are intended to trigger certain actions like copying, faxing or mailing the scanned document.
This daemon monitors the scanner's buttons and runs a shell script whenever one
of these buttons has been pressed. Because it is accessing the scanner directly
via libusb, there should be no conflicts with SANE or other scanner drivers:
scanbuttond simply won't touch the scanner hardware while you are using SANE.

Comment 1 Parag AN(पराग) 2006-10-03 06:18:33 UTC
I need help rpmlint is not silent
 rpmlint -iv scanbuttond-0.2.3-2.src.rpm 
I: scanbuttond checking
W: scanbuttond strange-permission scannerbuttond 0755
A file that you listed to include in your package has strange
permissions. Usually, a file should have 0644 permissions.

W: scanbuttond strange-permission buttonpressed.sh 0755
A file that you listed to include in your package has strange
permissions. Usually, a file should have 0644 permissions.

W: scanbuttond strange-permission initscanner.sh 0755
A file that you listed to include in your package has strange
permissions. Usually, a file should have 0644 permissions.

But all those 3 files are shell script files and to make them executable they
must have 0755 mode.

AND

rpmlint -iv scanbuttond-0.2.3-2.i386.rpm 
I: scanbuttond checking
Traceback (most recent call last):
  File "/usr/share/rpmlint/rpmlint.py", line 268, in ?
    main()
  File "/usr/share/rpmlint/rpmlint.py", line 92, in main
    runChecks(pkg)
  File "/usr/share/rpmlint/rpmlint.py", line 149, in runChecks
    c.check(pkg)
  File "/usr/share/rpmlint/InitScriptCheck.py", line 73, in check
    fd=open(pkg.dirName() + '/' + f, 'r')
IOError: [Errno 21] Is a directory



Comment 2 Jima 2006-10-03 17:52:31 UTC
Taking ownership of this bug, blocking FE-REVIEW.

Not doing a review yet; let's fix a couple bugs first.

First set of errors: chmod those files to 644 before generating your SRPM.  They
don't need to be executable before the build process puts them into
%{buildroot}.  That's what rpmlint is complaining about.

The second set of errors stems from the fact that you installed the initscript
as /etc/rc.d/init.d/scannerbuttond/scannerbuttond, not
/etc/rc.d/init.d/scannerbuttond -- you created the directory
%{buildroot}%{_initrddir}/scannerbuttond when you should have created
%{buildroot}%{_initrddir} .  Easy fix.  That allows rpmlint to run successfully
on the main package, albeit with output:

E: scanbuttond incoherent-subsys /etc/rc.d/init.d/scannerbuttond scanbuttond
E: scanbuttond incoherent-subsys /etc/rc.d/init.d/scannerbuttond scanbuttond
E: scanbuttond incoherent-subsys /etc/rc.d/init.d/scannerbuttond scanbuttond
W: scanbuttond incoherent-init-script-name scannerbuttond

The first three have to do with a mismatch between your initscript's name
(scannerbuttond) and the lock file in /var/lock/subsys/ (scanbuttond).  The last
one has to do with a similar mismatch between the package name and the
initscript.  Is there a particular reason you'd like the initscript to be named
scannerbuttond when every other reference says scanbuttond?  If so, fix the
initscript to use the right lockfile name (and I guess the warning isn't a
blocker).  If not, change the initscript name and all the errors go away.

I hacked together a fixed spec, which you can peruse here:

http://beer.tclug.org/fedora-extras/misc/scanbuttond.spec

In addition to using that spec, I chmod'd Source1-3 644 and renamed
scannerbuttond to scanbuttond.init for clarity.

On another (minor) stylistic note, I suspect, judging by the nature in which
they get installed, that Source2 & Source3 could possibly be formed as a patch
instead.

Yep.  Shaves a couple lines off your spec, and a measly ~500 bytes off.  No huge
deal.  I won't consider your method to be a blocker, by any means.

I'll do a more thorough review once you remedy the actual problems.

Comment 3 Jima 2006-10-03 18:11:58 UTC
Heh, don't worry about the Source2-3 vs. Patch0 thing, looks like it'd only
shave 129 bytes off the SRPM.  Were the files bigger, it might be worthwhile.

Comment 4 Parag AN(पराग) 2006-10-04 06:02:35 UTC
(In reply to comment #2)
> Taking ownership of this bug, blocking FE-REVIEW.
thanks.

> 
> Not doing a review yet; let's fix a couple bugs first.
> 
> First set of errors: chmod those files to 644 before generating your SRPM.  They
> don't need to be executable before the build process puts them into
> %{buildroot}.  That's what rpmlint is complaining about.
Done.

> 
> The second set of errors stems from the fact that you installed the initscript
> as /etc/rc.d/init.d/scannerbuttond/scannerbuttond, not
> /etc/rc.d/init.d/scannerbuttond -- you created the directory
> %{buildroot}%{_initrddir}/scannerbuttond when you should have created
> %{buildroot}%{_initrddir} .  Easy fix.  That allows rpmlint to run successfully
> on the main package, albeit with output:
> 
> E: scanbuttond incoherent-subsys /etc/rc.d/init.d/scannerbuttond scanbuttond
> E: scanbuttond incoherent-subsys /etc/rc.d/init.d/scannerbuttond scanbuttond
> E: scanbuttond incoherent-subsys /etc/rc.d/init.d/scannerbuttond scanbuttond
> W: scanbuttond incoherent-init-script-name scannerbuttond
> 
> The first three have to do with a mismatch between your initscript's name
> (scannerbuttond) and the lock file in /var/lock/subsys/ (scanbuttond).  The last
> one has to do with a similar mismatch between the package name and the
> initscript.  Is there a particular reason you'd like the initscript to be named
> scannerbuttond when every other reference says scanbuttond?  If so, fix the
> initscript to use the right lockfile name (and I guess the warning isn't a
> blocker).  If not, change the initscript name and all the errors go away.
ok.

> 
> I hacked together a fixed spec, which you can peruse here:
> 
> http://beer.tclug.org/fedora-extras/misc/scanbuttond.spec
> 
> In addition to using that spec, I chmod'd Source1-3 644 and renamed
> scannerbuttond to scanbuttond.init for clarity.
Thanks for solving rpmlint errors.

> 
> On another (minor) stylistic note, I suspect, judging by the nature in which
> they get installed, that Source2 & Source3 could possibly be formed as a patch
> instead.
  I also thought same first. But then i decided to have more flexibility to end
user in terms of let user write own buttonpressed.sh script and rebuild binary
RPM. Also he can install my provided script and later on installing binary rpm
he can change that script.
  Need your suggestions whether to create a patch or let package as it is now.

> 
> Yep.  Shaves a couple lines off your spec, and a measly ~500 bytes off.  No huge
> deal.  I won't consider your method to be a blocker, by any means.
> 
> I'll do a more thorough review once you remedy the actual problems.

here is updated package 
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-3.src.rpm

Comment 5 Jima 2006-10-04 11:43:06 UTC
> Thanks for solving rpmlint errors.

No problem.  They had me pretty confused, too, so don't feel too bad.  I ended
up extracting the files using `rpm2cpio scanbuttond-0.2.3-2.fc6.i386.rpm | cpio
-id` and just eyeballing things.  (`rpm -qpl` *should* have given me enough
hint, but I wasn't seeing it.)  I knew there had to be something odd to throw
off rpmlint. :-)

> Need your suggestions whether to create a patch or let package as it is now.

Nah, your methodology is better for this case, especially with the reasoning
you're using.  My chain of thought was a bit idealistic.

The only other thing that comes to mind (mainly by your idea to let the user
provide their own buttonpressed.sh script) is that you might want to explicitly
refer to that file with a %config(noreplace) tag, so that if the user edits the
file after installation, it doesn't get blown away, should there be an update
(or they upgrade to the next Core release).  Don't worry about it right now;
wait until after I do a full work (once I get to the office today).  Just
something to consider.

Also, you don't need to attribute my changes to me in your final spec; I only
updated the changelog to ennumerate what I changed. :-)

More in a few hours, thanks.

Comment 6 Parag AN(पराग) 2006-10-04 12:46:39 UTC
Thanks for your reply.
Here is updated package.
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-3.src.rpm

Also i tried to make buttonpressed.sh as %config(noreplace) but i got rpmlint
warning as
E: scanbuttond executable-marked-as-config-file /etc/scanbuttond/buttonpressed.sh
Executables must not be marked as config files because that may
prevent upgrades from working correctly. If you need to be able to
customize an executable, make it for example read a config file in
/etc/sysconfig.

Looks like this package is now ready for Full Review

Comment 7 Jima 2006-10-04 13:46:38 UTC
Yep, that it is.  Kind of annoying about executable-marked-as-config-file;
thanks for taking that suggestion into consideration (rpmlint showed me, I guess!).

Now for the review you earned. :-)

Using my own review checklist:
http://beer.tclug.org/fedora-extras/review-checklist-1.1.txt

1. rpmlint returns no output for any of the RPMs/SRPM.
2. Package is named as per the Package Naming Guidelines.
3. Spec is scanbuttond.spec
4. Package appears to meet the Packaging Guidelines.
5. Project site listed package as GPL'd.
6. License tag in spec concurs.
7. COPYING included in %doc.
8. Spec file is written in American English.
9. Spec file is legible.
10. Tarball matches upstream, although see below.
11. Package builds fine on FC-5/i386, FC-5/ppc and devel/i386.
12. n/a, unless it doesn't build on x86_64.
13. Package builds in Plague, so I suspect all BuildReqs are fulfilled.
14. Package does not handle locales either properly or improperly.
15. Package runs ldconfig in %post/%postun.  However, it doesn't have
appropriate Requires(...) lines for /sbin/ldconfig.  Please add.
16. Package is not designed to be relocatable.
17. Package does not own /etc/scanbuttond/ -- the change you made in the last
iteration removed ownership of this directory.  (Sorry, partly my doing.)
18. No duplicate entries in %files.
19. Permissions seem sane; %files has %defattr(...) line.
20. Package has valid %clean section.
21. Macro use appears rather consistent.
22. Package contains code, not content.
23. %doc is small; no -doc subpackage required.
24. %doc does not affect runtime.
25. n/a, no headers or static libraries.
26. n/a, no .pc files.
27. -devel subpackage contains .so files, check.
28. -devel subpackage properly requires the main package.
29. n/a, no .la files.
30. n/a, no GUI app.
31. Package does not own directories/files owned by other packages.
32. Release tag contains %{?dist}.
33. n/a, already includes COPYING.
34. n/a, no translations available, I imagine.
35. Package builds in Plague.
36. I can't verify x86_64, but package builds on i386 and ppc.
37. I'd verify that the package functions as described, but I need a new power
supply for my scanner.
38. Unless I'm mistaken, I believe (most) scriptlet commands are supposed to end
with "|| :" -- aha:

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-1f750660fb098f31a6a55cb58e752c53dcae39e4

Your 'service scanbuttond condrestart' has it, but 'service scanbuttond stop' is
missing the ||; the rest should be fine as-is.  (ldconfig and chkconfig
seem to be excluded.)

39. n/a, no subpackages besides -devel.

As a side note, I'm not sure if it's a blocker, but you only refer to the
filename for Source0.  I believe it's supposed to be a URL.  As it's a
Sourceforge-hosted project, it'd likely be one of:

http://easynews.dl.sourceforge.net/sourceforge/scanbuttond/scanbuttond-0.2.3.tar.gz

(or another mirror) or

http://dl.sf.net/scanbuttond/scanbuttond-0.2.3.tar.gz

Either of which can be simplified in maintenance with well-placed %{name} &
%{version} tags. :-)

So, as far as I can see, address #10, #15, #17, and #38, and I think we have a go.

Comment 8 Parag AN(पराग) 2006-10-05 04:46:42 UTC
Thanks for pointing out missings
Here is updated package.
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-3.src.rpm

Comment 9 Parag AN(पराग) 2006-10-09 08:08:17 UTC
Jima,
Can you check new SRPM uploaded for anything still missings?

Comment 10 Jima 2006-10-09 13:25:30 UTC
Sorry about that...

First off, please bump the Release number whenever you make changes (as you did,
apparently, but your SRPM link was still to -3).

Requires(preun): /sbin/ldconfig

should be (I think)

Requires(postun): /sbin/ldconfig

You're running ldconfig in %postun, not %preun.

Your other three fixes are correct, though.

Comment 11 Parag AN(पराग) 2006-10-10 04:57:30 UTC
here it goes final version
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-5.src.rpm

Comment 12 Ralf Corsepius 2006-10-10 06:35:48 UTC
Parag, I regret having to say this, but this package's packaging is pretty broken:

MUSTFIX:

/usr/bin/scanbuttond dlopens the  libscanbtnd-backend* libraries by name, using
the name libscanbtnd-backend*.so (Note: *.so, not *.so.1) rsp. by the name
specified in meta.conf (All set to *.so)

C.f. scanbuttond --help
and meta.conf inside of the sources.

Consequences:

1. Though the libscanbtnd-backend libs actually are shared libs, they are being
used as plugins to be dlopened by name, and not as normal shared libs.

=> Shipping the *.so.* in %{_libdir} doesn't make much sense.
=> The *.so must be part of the same package as scanbuttond, or the sources need
 to be patched to dlopen the *.so.1 instead of the *.so.

2. The *-devel package doesn't make sense, because the package does not provide
an API (header files) nor an API to load the plugins (it is hidden as private
files inside of scanbuttond.c and lib/loader.c)

Recommendation:
1. Remove the *-devel package

2. %configure --libdir=%{_libdir}/scanbuttond
This will cause the *.so* to land in %{_libdir}/scanbuttond instead of %{_libdir}

3. ship all %{_libdir}/scanbuttond/lib*.so as part of the main package.

4. Consider to modify the sources to dlopen(*.so.1) instead of *.so


Comment 13 Parag AN(पराग) 2006-10-10 07:05:00 UTC
hi Ralf,
any hints how to do following?
Consider to modify the sources to dlopen(*.so.1) instead of *.so

Comment 14 Ralf Corsepius 2006-10-10 15:35:06 UTC
Created attachment 138152 [details]
Proposed spec changes

Comment 15 Ralf Corsepius 2006-10-10 15:36:05 UTC
Created attachment 138153 [details]
let scanbuttond dlopen *.so.1

Comment 16 Ralf Corsepius 2006-10-10 15:37:09 UTC
(In reply to comment #13)
> any hints how to do following?
> Consider to modify the sources to dlopen(*.so.1) instead of *.so
cf. the patches I just attached.


Comment 17 Parag AN(पराग) 2006-10-11 04:33:21 UTC
I created a new SRPM from those patches
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-6.src.rpm



Comment 18 Jima 2006-10-11 15:02:33 UTC
The only problem I'm seeing is that the package doesn't own
%{_libdir}/scanbuttond .  Otherwise, looks good.

I only this morning realized that it doesn't support my scanner.  Rats!  Oh
well. :-)

Comment 19 Parag AN(पराग) 2006-10-12 04:19:50 UTC
Updated package 
Spec URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond.spec
SRPM URL: http://people.redhat.com/pnemade/scanbuttond/scanbuttond-0.2.3-7.src.rpm

BTW which scanner you have?

Comment 20 Jima 2006-10-12 14:06:08 UTC
Looks good to me.  I think we have an APPROVED package.

I have an HP PSC1350.  Judging by the lack of an HP backend library, I guess
this isn't the answer for me.  (Of course, I haven't looked -- there may very
well be a different software package that supports it.)

Comment 21 Parag AN(पराग) 2006-10-12 16:22:34 UTC
thanks very much for approving package.
Will import on Monday as i am on vacation now. Then will close this bug.
yes HP backend is not there. But it can be worked provided developers should get
some HP scanners to work on.
Anyway thanks for your time.


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