[Bug 655457] Review Request: codemirror - In-browser code editing made bearable

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=655457

--- Comment #1 from Michael Schwendt <mschwendt at gmail.com> 2010-12-23 05:43:49 EST ---
A brief look at the spec file (no full review):


> #==================
> # Macro definitions
> #==================
> %global url          http://codemirror.net/
> %global pub          http://aikiframework.org/files/codemirror/
> %global name         codemirror
> %global version      0.91
> %global revision     1
> %global pkgdist      %{name}-%{version}.zip
> %global pkgdistdir   CodeMirror-%{version}
> %global httpdconf    %{name}.conf
> %global httpdconfdir %{_sysconfdir}/httpd/conf.d
> %global httpdservice /sbin/service httpd
> 
> #====================
> # Package definitions
> #====================
> Name:      %{name}
> Version:   %{version}
> Release:   %{revision}%{?dist}
> Summary:   In-browser code editing made bearable
> Group:     Applications/Internet
> License:   zlib
> URL:       %{url}
> Source0:   %{url}/%{pkgdist}
> Source1:   %{httpdconf}
> BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> BuildArch: noarch

In my opinion, these two sections are a superfluous overuse of variables. I
highly recommend trimming down the spec preamble to improve readability and to
remove ambiguities.

The spec file is "huge enough" because of many subpackages that during update
preparations it will be necessary to spend time on (re-reviewing) the
individual subpackage definition sections anyway (e.g. the various licences and
URLs). Having to deal with macros that are used only once, e.g. %pub, isn't
helpful. The various licenses could be summed up near the top of the spec, btw.


> %global name         codemirror
> Name:      %{name}

The "Name:" tag already defines %name, so what you do here is redundant. Simply

  Name: codemirror

near the top of the spec file would be very clear and sufficient.


> %global version      0.91
> Version:   %{version}

Same here. Plus, you'll get into trouble whenever you will need to apply
special pre-release/post-release versioning schemes (such as Fedora's).


> %global revision     1
> Release:   %{revision}%{?dist}

Worse even. It would be necessary to modify both lines for a least-significant
%release bump, such as  2%{?dist}.1  Apart from that, %revision is not used
elsewhere, so why introduce it at all? "Release:" already defines %release.


> BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Note that several details related to BuildRoot are not needed anymore in Fedora
13 and newer: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> #=============
> # Requirements
> #=============
> Requires: httpd

The fat header is superfluous. Better add a brief comment that explains this
explicit dependency on "httpd", and possibly answer why a dependency on the
virtual "webserver" package wouldn't work.

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

That Wiki section focuses on library Requires a bit, but also applies to
explicit Requires in general.


> %package doc
> Summary:  CodeMirror documentation
> Group:    Documentation
> Requires: %{name} = %{version}-%{release}

Here, a comment would explain why a Documentation package needs the base
package. I see the some of the HTML pages refer to .js files on localhost, so
that's sort of what a comment in the spec should cover.


> %{__mkdir} -p -m 0755 %{buildroot}%{_defaultdocdir}/%{name}-%{version}
> %{__install} -p -m 0644 *.html %{buildroot}%{_defaultdocdir}/%{name}-%{version}

Caution! Manually filling %_defaultdocdir/%{name}-%{version} with files
conflicts with %doc. It's a pitfall that packagers run into regularly during
review. If in a later release, a %doc line is added to the files list for
%name, it would lead to killing/overriding the contents of the manually filled
directory. Possible work-arounds: 1) Using a different docdir. 2) Adding a
warning to the %files list that %doc MUST NOT be used.


> #=============
> # Post process
> #=============
> %post
> %{httpdservice} condrestart &> /dev/null || :
> %postun
> %{httpdservice} condrestart &> /dev/null || :
> 
> #==========================
> # Files included in package
> #==========================

These fat headers here bite you. Examine the output of:
rpm -qp --scripts codemirror-0.91-1.fc14.noarch.rpm

This has bitten other packagers before, even worse when using non-Shell
scriptlet sections.


> $ rpmls -p codemirror-0.91-1.fc14.noarch.rpm|grep ^d
> $

https://fedoraproject.org/wiki/Packaging:UnownedDirectories

For example, directory entries /usr/share/codemirror/ and
/usr/share/doc/codemirror-0.91/ are not included in the package. Mistake may
apply to other subpackages, too.


> # http://svn.exoplatform.org/projects/gwt/trunk/exo-gwtframework-editor/src/main/resources/org/exoplatform/gwtframework/editor/public/codemirror/css/groovycolors.css
> Source4:  groovycolors.css

Wouldn't it be much more convenient to use the full download URL directly in
the Source tag? Especially if you don't rename the file. I see that in some of
the sources, you rename the downloaded file to avoid conflicts. How about
adding a tiny shell script that as a separate Source file, which can be run to
automate the wget downloading and renaming of all Source files you need? And
optionally show a diff compared with previously download files. It would make
package maintenance easier.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.