Bug 501101

Summary: Review Request: emacs-color-theme - elisp mode to customize emacs look and feel
Product: [Fedora] Fedora Reporter: Filippo Argiolas <fargiolas>
Component: Package ReviewAssignee: Jonathan Underwood <jonathan.underwood>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, ionut, jonathan.underwood, notting, npajkovs, sagarun, xma
Target Milestone: ---Flags: jonathan.underwood: fedora-review+
j: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: emacs-color-theme-6.6.0-4.el6 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-05-12 18:14:27 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:

Description Filippo Argiolas 2009-05-16 07:33:36 UTC
Spec URL: http://people.freedesktop.org/~fargiolas/color-theme/emacs-color-theme.spec
SRPM URL: http://people.freedesktop.org/~fargiolas/color-theme/emacs-color-theme-6.6.0-1.fc11.src.rpm
Description: 
color-theme is an emacs mode that provides a lot of different presets to customize emacs faces. It also includes a little framework to easily create new themes from your own customizations and submit them to the maintainer.
Check http://www.nongnu.org/color-theme/ and http://www.emacswiki.org/emacs/ColorTheme for more informations.

This is my first package, usually I'm not too much interested into packaging stuff, but this is one of the first things I always install right after the distribution, so it shouldn't be hard to keep the spec file up to date.
Anyway, I'm also fine if someone else wants to take its maintainership for me (it's pretty trivial, I'm not sure it's worth to go into the whole sponsorship, account requesting thing just for this tiny package :P).

Comment 1 Arun S A G 2009-09-23 05:53:56 UTC

#1  Rpmlint on emacs-color-theme-6.6.0-1.fc11.noarch.rpm throws

emacs-color-theme.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/emacs-color-theme-6.6.0/COPYING

Please check https://fedoraproject.org/wiki/Common_Rpmlint_issues#wrong-script-end-of-line-encoding

#2 Source0 should point to http://ftp.twaren.net/Unix/NonGNU/color-theme/color-theme-6.6.0.tar.gz

source0: http://ftp.twaren.net/Unix/NonGNU/color-theme/%{pkg}-%{version}.tar.gz

Please check http://fedoraproject.org/wiki/Packaging/SourceURL


Tried installing the rpm it works fine;

The following lines were added to .emacs  file and i success fully loaded robin-hood theme
(require 'color-theme)
(color-theme-initialize)
(color-theme-robin-hood)


I can maintain this package for you if you want;

Comment 2 Filippo Argiolas 2009-09-23 07:16:20 UTC
(In reply to comment #1) 
> #1  Rpmlint on emacs-color-theme-6.6.0-1.fc11.noarch.rpm throws
> emacs-color-theme.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/emacs-color-theme-6.6.0/COPYING
> #2 Source0 should point to
> source0: http://ftp.twaren.net/Unix/NonGNU/color-theme/%{pkg}-%{version}.tar.gz

Ok, everything should be fixed now. Check the new files in http://people.freedesktop.org/~fargiolas/emacs-color-theme-fixed/

> I can maintain this package for you if you want;

Sure, if you already have packager privileges, go ahead. Thank you!

Comment 3 Arun S A G 2009-09-24 11:10:23 UTC
> Ok, everything should be fixed now. Check the new files in
> http://people.freedesktop.org/~fargiolas/emacs-color-theme-fixed/


Yes, rebuilt and checked no issues.

> Sure, if you already have packager privileges, go ahead. Thank you!  

I am not a packager yet, waiting for sponsorship.

Comment 4 Xavier Maillard 2009-10-20 20:24:32 UTC
where are the files ? I got 404 for every links here

Comment 5 Filippo Argiolas 2009-10-21 09:38:49 UTC
(In reply to comment #4)
> where are the files ? I got 404 for every links here  

There has been a big power supply failure at freedesktop.org about a month ago and there were no backups for user home dirs. I'll try to reupload the files this evening or tomorrow.

Comment 6 Filippo Argiolas 2009-10-28 07:41:13 UTC
Sorry for the delay, exams time. Here you should find the missing files:
http://www.gnome.org/~fargiolas/emacs-color-theme/
That's the most updated files I found on my hard disk, I'm almost sure they are the fixed ones and rpmlint seems to confirm it.

Comment 7 Xavier Maillard 2009-10-30 14:02:26 UTC
Got' em. Installed, worked. Pretty.

Do you still need a sponsor ?

Comment 8 Jonathan Underwood 2010-04-06 01:17:52 UTC
Rebuilding packages in mock works fine. rpmlint output on resulting packages:

$ rpmlint *.rpm
emacs-color-theme.noarch: W: spelling-error %description -l en_US customizations -> customization, customization's, customization s
emacs-color-theme.src: W: spelling-error %description -l en_US customizations -> customization, customization's, customization s

----> These can be ignored.

emacs-color-theme.src:15: W: mixed-use-of-spaces-and-tabs (spaces: line 14, tab: line 15)

-----> This needs fixing (untabify the spec file)

emacs-color-theme-el.noarch: W: spelling-error Summary(en_US) Elisp -> Lisp, Elise, Elisa
emacs-color-theme-el.noarch: W: spelling-error %description -l en_US elisp -> lisp, e lisp, Elise

------> Ignore these

emacs-color-theme-el.noarch: W: no-documentation


-------> This is fine, ignore.
3 packages and 0 specfiles checked; 0 errors, 6 warnings.

Comment 9 Jonathan Underwood 2010-04-06 01:42:05 UTC
The package needs updating to comply with the most recent emacs packaging guidelines: http://fedoraproject.org/wiki/Packaging:Emacs

Specifically, in the spec file:

1/ Remove the pkgconfig stuff

2/ Update the macros to use the new names eg %{emacs_version}->{_emacs_version} etc

3/ Don't buildrequire emacs-el

4/ Add comments about the patches - have they been sent upstream, if so when. Give URLs to upstream bugzilla or mailing list archives if appropriate

5/ Spec file legibility is compromised by having lines commented out, particularly ones which begin with macros - commenting a macro may not always disable it! Please remove the unneeded commented code lines from the spec file.

In addition, note that color-theme is already included in the emacs-goodies package which is already in Fedora. Personally I would like to see color-theme packaged separately from the goodies collection (which contains a lot of nasty elsip of low quality). Once this passes review you'll need to coordinate with the emacs-goodies package owner to remove the color theme stuff from that package.


Formal Review:
Key: 
[X] All is OK.
[A] Needs action


[A] MUST: rpmlint must be run on every package. The output should be posted in the review.

--->See comment #8 for things that need fixing.

[X] MUST: The package must be named according to the Package Naming Guidelines .
[X] MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
[A] MUST: The package must meet the Packaging Guidelines .
---> See the points above
[X] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[A] MUST: The License field in the package spec file must match the actual license.
--->color-theme.el states that the files is GPLv2+, but the spec file says GPLv2. This needs clarifying.
[X]MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4]
[X] MUST: The spec file must be written in American English. [5]
[A] MUST: The spec file for the package MUST be legible. [6]
---> See point above about removing the commented out lines

[X] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
Packaged source md5sum:  
     a4de73c236a6af11ab378bfe18dabcca  color-theme-6.6.0.tar.gz
Upstream md5sum:
     a4de73c236a6af11ab378bfe18dabcca  color-theme-6.6.0.tar.gz
[X] MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]
[X] MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]
[X] MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
[X] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
[X] MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]
[X] MUST: Packages must NOT bundle copies of system libraries.[11]
[X] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12]
[X] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]
[X] MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [14]
[X] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [15]

[A] MUST: Each package must consistently use macros. [16]
----> Please use the correct macros from the most recent Emacs add-on packaging guidelines

[X] MUST: The package must contain code, or permissable content. [17]
[X] MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]
[X] MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18]
[X] MUST: Header files must be in a -devel package. [19]
[X] MUST: Static libraries must be in a -static package. [20]
[X] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [19]
[X] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [21]
[X] MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20]
[X] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [22]
[X] MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [23]
[X] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [24]
[X] MUST: All filenames in rpm packages must be valid UTF-8. [25]



[X] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [26]
[X] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [27]
[X] SHOULD: The reviewer should test that the package builds in mock. [28]
[X] SHOULD: The package should compile and build into binary rpms on all supported architectures. [29]
[X] SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
[X] SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [30]
[X] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [21]
[X] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [31]
[X] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [32]
[X] SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.[33]

Comment 10 Filippo Argiolas 2010-04-17 15:48:46 UTC
Unfortunately, I have very little time to dedicate to this thing, if anyone wants to pick up where I've left I'll be more than glad!
Otherwise I will probably look at the pointed issues but not anytime soon.
Answering some questions: 
- patches weren't upstreamed
- emacs-goodies didn't exist at the time I wrote the spec file. Arun, would you like to split color-themes from your package and maintain an independent package for it?

Comment 11 Arun S A G 2010-04-17 18:25:35 UTC
Filippo, I will remove color-themes from emacs-goodies in next update. Let me pick up from where you left :-)

Spec URL: http://sagarun.fedorapeople.org/SPECS/emacs-color-theme.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/emacs-color-theme-6.6.0-2.fc12.src.rpm

Comment 12 Jonathan Underwood 2010-04-17 20:06:27 UTC
Arun, great to see you're picking the package up, and that you've tackled all the points in the review.

A few small things:

1/ For F-13 onwards, the %clean section is unnecessary: 
https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean

2/ The Buildroot tag isn't needed, and you don't need to do rm -rf %{buildroot}
in %install:
https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

3/ The patches still need a spec file comment regarding their upstream status:
https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Also - should this package not have a startup file (in %{_emacs_sitestartdir}) ?

Comment 13 Jonathan Underwood 2010-05-09 11:31:44 UTC
Hi Arun - any update on this?

Comment 14 Arun S A G 2010-05-09 12:52:19 UTC
Hi,

I will update this package in upcoming week.

Comment 16 Jonathan Underwood 2010-05-11 17:10:01 UTC
OK, great. 6.6.0-3 builds fine in mock, passes review, and functions properly.

APPROVED.

Comment 17 Arun S A G 2010-05-11 17:17:35 UTC
New Package CVS Request
=======================
Package Name:emacs-color-theme
Short Description: Color themes for Emacs
Owners: sagarun
Branches: F13

Comment 18 Kevin Fenzi 2010-05-12 17:12:43 UTC
CVS done (by process-cvs-requests.py).

Comment 19 Arun S A G 2010-06-15 15:51:07 UTC
New Package CVS Request
=======================
Package Name:emacs-color-theme
Short Description: Color themes for Emacs
Owners: sagarun
Branches: F12  

(I need to push this package for F12, Kindly create a F12 branch)

Comment 20 Jason Tibbitts 2010-06-16 22:04:15 UTC
Please file change requests to add new branches to existing packages; to do otherwise confuses the automation we have in place.

http://fedoraproject.org/wiki/CVS_admin_requests#Package_Change_Requests_for_existing_packages

I've created the new branch.

Comment 21 Arun S A G 2011-02-17 18:39:39 UTC
Package Change Request
======================
Package Name: emacs-color-theme
New Branches: el5 el6
Owners: sagarun

(I'd like to have this package on EPEL)

Comment 22 Jason Tibbitts 2011-02-17 18:45:05 UTC
Git done (by process-git-requests).

Comment 23 Fedora Update System 2011-02-19 11:57:52 UTC
emacs-color-theme-6.6.0-4.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/emacs-color-theme-6.6.0-4.el5

Comment 24 Fedora Update System 2011-02-21 16:28:34 UTC
emacs-color-theme-6.6.0-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/emacs-color-theme-6.6.0-4.el6

Comment 25 Fedora Update System 2011-03-07 18:23:12 UTC
emacs-color-theme-6.6.0-4.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 26 Fedora Update System 2011-03-09 18:27:32 UTC
emacs-color-theme-6.6.0-4.el6 has been pushed to the Fedora EPEL 6 stable repository.