Bug 1002328 - (nasacircle) Review request: grub2-circled-nasa-themes - A circled countdown on a NASA iotd wallpaper
Review request: grub2-circled-nasa-themes - A circled countdown on a NASA iot...
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Björn 'besser82' Esser
Fedora Extras Quality Assurance
:
Depends On: grub2-icons
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-28 18:31 EDT by Simon A. Erat
Modified: 2013-11-21 10:35 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
besser82: fedora‑review?


Attachments (Terms of Use)

  None (edit)
Description Simon A. Erat 2013-08-28 18:31:12 EDT
Heyas

Just completed my first cool GRUB2 theme, and i'd like to share with the community.

Its a spacebackground with OS-icons next to the menu entries, 
surrounded by a circle timeout-bar.

It contains icons which may be reused with GPL3.
It uses images from NASA, which may be used as GPLv3.
Source: http://www.nasa.gov/audience/formedia/features/MP_Photo_Guidelines.html
Partial quote: "NASA images may be used as graphic "hot links" to NASA Web sites, provided they are used within the guidelines above. This permission does not extend to use of the NASA insignia, the retired NASA logotype or the NASA seal."

SPEC: http://sea.fedorapeople.org/review/grub2-theme-nasacircle/nasacircle.spec
SRPM: http://sea.fedorapeople.org/review/grub2-theme-nasacircle/nasacircle-0.1-1.fc17.src.rpm

Have a good day/night
Comment 2 Björn 'besser82' Esser 2013-08-29 01:36:55 EDT
Hey Simon!

Please file review-request for new packages always against "Package Review" component.  Please be careful about that in future!  :)

I'll take this one.

Cheers,
  Björn
Comment 3 Ralf Corsepius 2013-08-29 02:48:02 EDT
Your spec file contains this:

%install
tar -axf %{sources}

2 issues with this:
- Sources are supposed to be unpackaged in %setup.

- Instead of explicitly invoking tar, sources should be unpackaged with %setup.
  Please google for "rpm %setup", rpm has options to control directory creation
  sequencing and untarring into subdirs.
Comment 4 Christopher Meng 2013-08-29 02:59:23 EDT
(OT)I think bugzilla has problem now. Readd CC
Comment 6 Björn 'besser82' Esser 2013-08-30 03:44:06 EDT
your spec-file has a severe error:

  %changelog
  * Wed Aug 29 2013 - Simon A. Erat (sea) @ sea@fedorapeople.org - 0.2-2
  - Fixed %prep / %install in spec

the last line from %changelog contains unescaped macros, which will make the build-process fail!  It MUST be like this:

  - Fixed %%prep / %%install in spec

Note the doubled percent-symbol (%%) which is used to escape macros inside spec-file properly.  :)

Please fix this.
Comment 7 Mads Kiilerich 2013-08-31 13:05:48 EDT
First, regarding upstream:

It is normal good style that a X.tar.gz extracts in a folder named X.

It is confusing that the upstream project is called one thing and the downloadable file something else ... and the name of the actual themes something third. 

It seems like you are distributing font files without license.

It is claimed that the license is GPL, but that is not clear from the distributed source.

It seems like most of the files are the same for all the themes. Consider storing them in a "common" folder.

(It would be nice if upstream showed some screenshots so we had an idea what to expect.)


It seems like the name of this review request is wrong - it doesn't match the package name.

The prjapppath define do not help readability and it is far from obvious what it is short for. I would just use a wildcard in %files and live with having to specify /boot/grub2 a couple of times.

Do you really think the spec is finished? No summary, no description, a "Place compile commands here" placeholder. Have you tried running rpmlint on the spec?

Remove comment lines that doesn't apply.

There is no reason to clean buildroot in %install.

theme.txt is not a %doc file - but license files are.

"icons" looks like something that should be packaged in a separate subpackage ... or perhaps as a completely separate package that could be reused elsewhere. 

Fonts should reuse existing files in Fedora ... and pf2 files should be built from "source".
Comment 8 Christopher Meng 2013-09-04 20:15:36 EDT
ping here.
Comment 9 Simon A. Erat 2013-09-05 06:49:43 EDT
* Seperated icons as new package (https://bugzilla.redhat.com/show_bug.cgi?id=1004544)
* Fixed spec
* Fixed tarball

Font were created using grub2-mkfont, and i've chosen fonts from the default repos to use.
Solving one issue at a time, i hope the OS icons match the licenses now.

Screenshots: .. err... have to do them again, will place them on:
http://sea.fedorapeople.org/screenshots/grub2-circled-nasa-themes

SPEC: http://sea.fedorapeople.org/Review/grub2-circled-nasa-themes/grub2-circled-nasa-themes.spec
SRPM: http://sea.fedorapeople.org/Review/grub2-circled-nasa-themes/grub2-circled-nasa-themes-0.0.2-3.src.rpm
Comment 10 Christopher Meng 2013-11-14 02:31:13 EST
AH, are you going to support EL5?

And any news here, Björn?

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