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
Summary: | Review Request: scrot - Screen-shot capture using Imlib2 | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Michael Rice <michael> |
Component: | Package Review | Assignee: | Michael Schwendt <bugs.michael> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | pertusus |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-10-08 06:14:06 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Michael Rice
2006-09-28 14:41:26 UTC
This is my third package, and I am seeking a sponsor. * 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. 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 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) 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.
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? 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. How do you track what depends on what so that I can track these things before I submit in the future?? 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. * 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 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. |