Bug 660095 - Review Request: impressive - A program that displays presentation slides
Summary: Review Request: impressive - A program that displays presentation slides
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 558965 657127
TreeView+ depends on / blocked
 
Reported: 2010-12-05 14:24 UTC by Michael J Gruber
Modified: 2010-12-17 20:29 UTC (History)
4 users (show)

Fixed In Version: impressive-0.10.3-3.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-12-17 20:27:44 UTC
susi.lehtola: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Michael J Gruber 2010-12-05 14:24:18 UTC
Spec URL: http://mjg.fedorapeople.org/rpmdev/impressive.spec
SRPM URL: http://mjg.fedorapeople.org/rpmdev/impressive-0.10.3-1.fc14.src.rpm
Description:

Impressive is a program that displays presentation slides. But unlike
OpenOffice.org Impress or other similar applications, it does so with
style.

Smooth alpha-blended slide transitions are provided for the sake
of eye candy, but in addition to this, Impressive offers some unique tools
that are really useful for presentations.

Comment 1 Michael J Gruber 2010-12-05 14:26:25 UTC
Package "impressive" used to be available for F13 but is orphaned; it
got marked retired for F14 and rawhide (master) because of maintainer
inactivity.

I took over ownership for F13, synced with upstream and cleaned up the spec
file.

Scratch build is at:

Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=2643812

Once approved I plan to request an SCM change for f14, rawhide and epel6
branches.

The review request for the original package is in bug #484726 .

Comment 2 Susi Lehtola 2010-12-05 15:11:28 UTC
rpmlint output:
impressive.noarch: W: no-manual-page-for-binary python-impressive
impressive.src: W: no-buildroot-tag
2 packages and 0 specfiles checked; 0 errors, 2 warnings.

**

The summary would be better as
 "A program that displays presentation slides"

**

The install scenario is a bit odd. I'd probably just install impressive.py as %{_bindir}/impressive, but for some reason the past maintainer has implemented a wrapper for checking that the hardware has OpenGL acceleration.

The install of the wrapped python script as %{_bindir}/python-impressive seems a bit silly. I'd install it as %{python_sitelib}/impressive.py.

**

This is a Python package, so you should add BuildRequires: python-devel to make sure everything goes alright.

**


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK

MUST: The package must be named according to the Package Naming Guidelines. ~OK
- Upstream uses the upper-case name Impressive, also in the tarball. However, the program in the tarball is "impressive.py", which would point to a lower-case name.
- It's better to keep the name in lowercase, since the package already exists in Fedora.

MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
1fefb25db71ee322a59353de85ae00b4  Impressive-0.10.3.tar.gz
1fefb25db71ee322a59353de85ae00b4  ../SOURCES/Impressive-0.10.3.tar.gz

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. N/A
MUST: Permissions on files must be set properly. OK

MUST: Large documentation files must go in a -doc subpackage. N/A
- demo.pdf is quite large in comparison to the other files, but I guess this is OK.

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A

MUST: Desktop files are installed properly. N/A
- Although the application is graphical, it needs to be launched from the command prompt.

MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK
EPEL: Clean section exists. OK
EPEL: Buildroot cleaned before install. OK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A

Comment 3 Susi Lehtola 2010-12-05 15:12:50 UTC
Michael: the fedora_review flag is set by the reviewer, not the submitter.

Comment 4 Michael J Gruber 2010-12-05 15:41:07 UTC
> The summary would be better as
>  "A program that displays presentation slides"

I'm happy to change it (just wasn't sure how much to deviate from the old package).

> 
> **
> 
> The install scenario is a bit odd. I'd probably just install impressive.py as
> %{_bindir}/impressive, but for some reason the past maintainer has implemented
> a wrapper for checking that the hardware has OpenGL acceleration.

That is the recommendation for users of opengl-games-utils.

> 
> The install of the wrapped python script as %{_bindir}/python-impressive seems
> a bit silly. I'd install it as %{python_sitelib}/impressive.py.

Again, this is how other opengl-games-utils users (all of them games it seems) do it. I can change it, of course, but it would also mean injecting the path %{python_sitelib} into the wrapper script.

> This is a Python package, so you should add BuildRequires: python-devel to make
> sure everything goes alright.

This package does not build anything and does not use setup.py etc. (distutils). Maybe it should, but it doesn't...
It's a single standalone python file/program (save the wrapper).

And sorry for the flag, I must have mixed that up. (Or did I do this before?)

Comment 5 Susi Lehtola 2010-12-05 15:56:00 UTC
Okay, if it's the standard for opengl-games-utils, then that's okay..

Although, the only thing you'd need to modify is SOURCE1, since the stuff in opengl-game-utils doesn't care at all about impressive.

Actually there is a bug in the current implementation: the wrapper has
 APP=python-impressive
 checkDriOK $APP
which will print
 "Therefore python-impressive cannot run."
if DRI support is missing, when it actually should print 
 "Therefore impressive cannot run."

If you place the python script in %{python_sitelib}, you'll just have to change SOURCE 1 to e.g.

 #!/bin/bash
 . /usr/share/opengl-games-utils/opengl-game-functions.sh
 APP=impressive
 checkDriOK $APP
 exec @PYTHONSITELIB@/$APP.py "$@"

and you'll have to install it with
 sed "s|@PYTHONSITELIB@|%{python_sitelib}|g" %{SOURCE1} > %{buildroot}%{bindir}/impressive
but this isn't a problem anyway.

The added convenience of installing into python_sitelib is that RPM will compile the python file into bytecode, so execution will be (marginally) faster.

(In reply to comment #4)
> > This is a Python package, so you should add BuildRequires: python-devel to make
> > sure everything goes alright.
> 
> This package does not build anything and does not use setup.py etc.
> (distutils). Maybe it should, but it doesn't...
> It's a single standalone python file/program (save the wrapper).

Nonetheless, you need python-devel so that the macros are declared and so that the bytecode compilation takes place (IIRC). Doesn't hurt having it, anyhow.

Comment 6 Michael J Gruber 2010-12-05 16:23:36 UTC
New spec addressing all reviewer comments:

http://mjg.fedorapeople.org/rpmdev/impressive.spec
http://mjg.fedorapeople.org/rpmdev/impressive-0.10.3-2.fc14.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2645340

Also, editing bug title to match the new summary.

Comment 7 Susi Lehtola 2010-12-05 16:51:57 UTC
Final comments:

As SOURCE1 is a self-created file, please add a comment to the spec file about what it does. For instance

# Wrapper script for making sure hardware acceleration is available
Source1: %{name}.sh

**

Please don't mix %{name} and "impressive" in %files. Choose one and stick with it.

Also, please be more explicit in %files. Free use of wildcards can end up in unwanted things, for instance if something goes wrong in the install you might not detect it if you use a wildcard. Explicit specs are also easier to read (you see straight away what files are installed). When you have dozens of binaries, wildcards have their place.

 Thus, please replace
 %{_mandir}/man1/*
with the more explicit
 %{_mandir}/man1/impressive.1*
(or, if you want to use %{name} in %files, use %{name} here as well).

**

Please address these issues before git import.

This package has been APPROVED.

Comment 8 Michael J Gruber 2010-12-05 18:06:37 UTC
Package Change Request
======================
Package Name: impressive
New Branches: f14 el6
Owners: mjg

Taking over an orphaned+retired F13 package. f13 ownership taken over in pkgdb.

Changed summary.

The review request for the original package is in bug #484726

Comment 9 Jason Tibbitts 2010-12-06 16:12:30 UTC
This package already has an f14 branch, so I can't process that request as-is.  I'm guessing that you were asking for the package to be unretired, but if that's the case then it would have been helpful for you to say that.

I've unretired the f14 and master branches; you should log into pkgdb and claim them.  I will create the el6 branch for you now.

Comment 10 Jason Tibbitts 2010-12-06 16:12:46 UTC
Git done (by process-git-requests).

Comment 11 Michael J Gruber 2010-12-06 16:49:17 UTC
(In reply to comment #9)
> This package already has an f14 branch, so I can't process that request as-is. 
> I'm guessing that you were asking for the package to be unretired, but if
> that's the case then it would have been helpful for you to say that.

Well, that's why I said "Taking over an orphaned+retired F13 package". Sorry for avoiding the trigger word "unretire".

> I've unretired the f14 and master branches; you should log into pkgdb and claim
> them.  I will create the el6 branch for you now.

Thanks. Claimed them, pushing and building.

"unretiring" being some mix of the "new submission" (e.g. new bug) and "update" (e.g. existing repo) processes, I'm afraid I'll trip over the next wire shortly.

Comment 12 Michael J Gruber 2010-12-06 16:57:33 UTC
(In reply to comment #11)
> "unretiring" being some mix of the "new submission" (e.g. new bug) and "update"
> (e.g. existing repo) processes, I'm afraid I'll trip over the next wire
> shortly.

such as this one:

Building impressive-0.10.3-3.fc15 for dist-rawhide
Created task: 2647367
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=2647367
Watching tasks (this may be safely interrupted)...
2647367 build (dist-rawhide, /impressive:3209089b8a5eec9ff3720599e9353223d5312348): free
2647367 build (dist-rawhide, /impressive:3209089b8a5eec9ff3720599e9353223d5312348): free -> open (x86-14.phx2.fedoraproject.org)
  2647368 buildSRPMFromSCM (/impressive:3209089b8a5eec9ff3720599e9353223d5312348): free
  2647368 buildSRPMFromSCM (/impressive:3209089b8a5eec9ff3720599e9353223d5312348): free -> open (x86-05.phx2.fedoraproject.org)
  2647368 buildSRPMFromSCM (/impressive:3209089b8a5eec9ff3720599e9353223d5312348): open (x86-05.phx2.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
2647367 build (dist-rawhide, /impressive:3209089b8a5eec9ff3720599e9353223d5312348): open (x86-14.phx2.fedoraproject.org) -> FAILED: BuildError: package impressive is blocked for tag dist-f15
  0 free  0 open  1 done  1 failed

2647367 build (dist-rawhide, /impressive:3209089b8a5eec9ff3720599e9353223d5312348) failed

Maybe some part of the infrastructure is updated (allowing me to push and build) and other isn't (denying me to push the build). I'll try again later.

Comment 13 Fedora Update System 2010-12-09 07:56:16 UTC
impressive-0.10.3-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/impressive-0.10.3-3.fc14

Comment 14 Fedora Update System 2010-12-09 08:05:04 UTC
impressive-0.10.3-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/impressive-0.10.3-3.fc13

Comment 15 Fedora Update System 2010-12-09 21:56:40 UTC
impressive-0.10.3-3.fc13 has been pushed to the Fedora 13 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 impressive'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/impressive-0.10.3-3.fc13

Comment 16 Fedora Update System 2010-12-17 20:27:35 UTC
impressive-0.10.3-3.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2010-12-17 20:29:20 UTC
impressive-0.10.3-3.fc13 has been pushed to the Fedora 13 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.