Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 208424 - Review Request: scrot - Screen-shot capture using Imlib2
Summary: Review Request: scrot - Screen-shot capture using Imlib2
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-09-28 14:41 UTC by Michael Rice
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-10-08 06:14:06 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Michael Rice 2006-09-28 14:41:26 UTC
Spec URL: http://errr.fluxbox-wiki.org/fedora_stuff/scrot.spec
SRPM URL: http://errr.fluxbox-wiki.org/fedora_stuff/scrot-0.8-1.fc5.src.rpm
Description: 
A nice and straightforward screen capture utility implementing the dynamic loaders of imlib2. Very popular among *box users from it being very light weight.

Comment 1 Michael Rice 2006-09-28 17:32:39 UTC
This is my third package, and I am seeking a sponsor.

Comment 2 Michael Schwendt 2006-10-04 12:23:32 UTC
* Project home page URL is  http://linuxbrit.co.uk/scrot/
  (note the /scrot/ postfix)


* rpmlint scrot-0.8-1.fc5.src.rpm 
W: scrot non-standard-group Applications/Graphics

Better:  User Interface/X


* License is MIT -- not BSD.
  cf. http://www.opensource.org/licenses/mit-license.html


* rpmlint /home/qa/tmp/rpm/RPMS/scrot-0.8-1.i386.rpm
W: scrot non-standard-group Applications/Graphics
W: scrot no-version-in-last-changelog

Although rpmlint doesn't understand where you've put the "0.8"
version in the %changelog, you should add the release value in there,
too:

* Tue Sep 26 2006 Michael Rice <errr[AT]errr-online.com>
- 0.8-1
- Initial RPM release

Or:

* Tue Sep 26 2006 Michael Rice <errr[AT]errr-online.com> - 0.8-1
- Initial RPM release

Or:

* Tue Sep 26 2006 Michael Rice <errr[AT]errr-online.com> 0.8-1
- Initial RPM release


* Documentation duplicated because of a wrong %patch. Keep the
%doc lines in the spec, get rid of %_datadir/%name:

$ rpmls -p /home/qa/tmp/rpm/RPMS/scrot-0.8-1.i386.rpm
-rwxr-xr-x  /usr/bin/scrot
drwxr-xr-x  /usr/share/doc/scrot-0.8
-rw-r--r--  /usr/share/doc/scrot-0.8/AUTHORS
-rw-r--r--  /usr/share/doc/scrot-0.8/COPYING
-rw-r--r--  /usr/share/doc/scrot-0.8/ChangeLog
-rw-r--r--  /usr/share/doc/scrot-0.8/README
-rw-r--r--  /usr/share/doc/scrot-0.8/TODO
-rw-r--r--  /usr/share/man/man1/scrot.1.gz
drwxr-xr-x  /usr/share/scrot
-rw-r--r--  /usr/share/scrot/AUTHORS
-rw-r--r--  /usr/share/scrot/ChangeLog
-rw-r--r--  /usr/share/scrot/README
-rw-r--r--  /usr/share/scrot/TODO


* ./src/Makefile.am sets a bad INCLUDES line. First of all, standard
search path for headers should not be re-added with -I as this breaks
customised search paths. And second, be careful that in updates (or
other packages) the -O3 does never come after our global optflags.


Comment 3 Michael Rice 2006-10-04 17:42:28 UTC
Michael, thanks for going over this package for me. I have redone some things,
and now rpmlint is no longer giving any output. I think I have fixed all the
things you pointed out with the exception of 

* ./src/Makefile.am sets a bad INCLUDES line. First of all, standard
search path for headers should not be re-added with -I as this breaks
customised search paths. And second, be careful that in updates (or
other packages) the -O3 does never come after our global optflags.

I am not sure exactly what to do here with the Makefile.am  I also have no idea
what you mean about the -O3 Would you be able to provide me with some more input?

Thanks.

http://errr.fluxbox-wiki.org/fedora_stuff/scrot/2/scrot.spec
http://errr.fluxbox-wiki.org/fedora_stuff/scrot/2/scrot-0.8-2.fc5.src.rpm

Comment 4 Patrice Dumas 2006-10-04 20:22:50 UTC
In src/(In reply to comment #3)
 
> I am not sure exactly what to do here with the Makefile.am  I also have no idea
> what you mean about the -O3 Would you be able to provide me with some more input?

If you look at the compilation, you'll notice that there is
   gcc -DHAVE_CONFIG_H -I. -I. -I. -g -O3 -Wall
and the RPM_OPT_FLAGS options on the same line 
   -O2 -g -pipe -Wall   -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
   -fstack-protector   --param=ssp-buffer-size=4 -m32 -march=i386 
   -mtune=generic -fasynchronous-unwind-tables
In that case the RPM_OPT_FLAGS (and specifically -O2) override the
other flags hardcoded in Makefile.am (and specifically -O3). That's
right, but the flags -g -O3 -Wall shouldn't have been set in the
first place. The upstream is broken, and should be notified that
those flags appearing in src/Makefile.am

INCLUDES          = -g -O3 -Wall -I/usr/X11R6/include \
$(X_CFLAGS) -I$(prefix)/include -I$(includedir) -I. \
-DPREFIX=\""$(prefix)"\" @GIBLIB_CFLAGS@

should be removed, (together with some -I which shouldn't be 
set here) such that the line looks like something along
   INCLUDES          = $(X_CFLAGS) -DPREFIX=\""$(prefix)"\" @GIBLIB_CFLAGS@
(or
   INCLUDES          = $(X_CFLAGS) -DPREFIX=\""$(prefix)"\" $(GIBLIB_CFLAGS)


Comment 5 Michael Schwendt 2006-10-04 21:10:54 UTC
Right. That was just a hint, a recommendation to be careful during
package review, since this can be an issue also with other packages.
And sometimes it changes unexpectedly when upstream modifies the
Makefile setup (e.g. our optflags might get overruled). Here it is
not a MUST FIX item. Just something to comment on.

> I/usr/X11R6/include -I/usr/include -I/usr/include

To expand on that, /usr/include is a standard search path for include
files. It need not be added to the user search list with -I again. It
is harmless with this package, but with other packages it can happen
that the added -I/usr/include enters packaged files and propagates to
other packages via build requirements. As a worst-case, the modified
search order for include files makes it impossible to customise the
search path further in order to locate the desired headers. In other
words, standard search locations suddenly override user-defined locations,
boom. This is a common mistake by some upstream projects.


Comment 6 Michael Rice 2006-10-05 00:52:23 UTC
So then is this package done or do I still need to do something else to it.. I
have reported this issue upstream for the software author to review it, anything
else or is this one at a stoping point?

Comment 7 Patrice Dumas 2006-10-05 07:41:36 UTC
The INCLUDES issue is not a blocker.

Regarding BuildRequires,
giblib-devel depends on imlib2-devel and imlib2-devel depends on
libX11-devel  

The README is useless since it contains only build instructions.

For the %description, I would have chosed what is on the home page, 
(in my opinion this is not a blocker):

   scrot is a commandline screen capture util like "import", but using 
   imlib2.

   It has lots of options for autogenerating filenames, and can do fun 
   stuff like taking screenshots of multiple displays and glueing them 
   together.

I don't have any other comment. The final word belong to Michael of 
course.

Comment 8 Michael Rice 2006-10-05 08:16:28 UTC
How do you track what depends on what so that I can track these things before I
submit in the future??

Comment 9 Patrice Dumas 2006-10-05 08:21:01 UTC
I just do rpm -q --requires giblib-devel.
Or I try to remove the package I suspect other packages depend 
on and I watch rpm error or yum remove dependencies resolution.

Comment 10 Michael Schwendt 2006-10-05 10:52:17 UTC
* Eliminating redundant BuildRequires is _not_ a MUSTFIX item and
sometimes can be tiresome and not worthwhile:

./src/scrot.h contains _direct_ dependencies on X headers, so an X
-devel rpm in the BuildRequires makes sense (even if it may be
redundant in the complete dependency-chain).

There is no direct dependency on Imlib2, since giblib is a wrapper
library for it and giblib-devel "Requires: imlib2-devel" already.
"BuildRequires: imlib2-devel" can be removed safely.


* I agree that the README in %doc can be deleted. It is irrelevant to
the rpm package user and hasn't changed in six years. Same applies to
the TODO file, which contains nothing of interest. These files can be
excluded after importing the package to CVS, though. And note that for
more active software projects, it might happen that an upgrade adds
interesting contents to either README or TODO ;) and a packager might
forget about re-adding them.


* APPROVED: scrot-0.8-2.fc5.src.rpm


Comment 11 Patrice Dumas 2006-10-05 13:17:53 UTC
I'll sponsor Michael. So, please Michael go through the steps 
to create a CLA. I hope you allready read
http://fedoraproject.org/wiki/Extras/Contributors

you should now proceed to
http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907
and I'll sponsor you.


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