Bug 655457

Summary: Review Request: codemirror - In-browser code editing made bearable
Product: [Fedora] Fedora Reporter: Steven Garcia <webwhammy>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: notting, package-review
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-04-24 19:50:23 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Steven Garcia 2010-11-20 21:35:40 EST
Spec URL: http://www.aikiframework.org/files/codemirror/codemirror.spec
SRPM URL: http://www.aikiframework.org/files/codemirror/codemirror-0.91-1.fc13.src.rpm

Description: CodeMirror is a JavaScript library that can be used to create a relatively pleasant editor interface for code-like content, computer programs, HTML markup, and similar.

This will be my third package review submission to Fedora and I
need a sponsor.

Ran successful rpmlint on spec, srpm and installed rpms
$ rpmlint codemirror* \
SPECS/codemirror.spec \
16 packages and 1 specfiles checked; 0 errors, 0 warnings.

Ran successful mock rebuild
mock --rebuild SRPMS/codemirror-0.91-1.fc13.src.rpm 
INFO: mock.py version 1.1.6 starting...
State Changed: init plugins
INFO: selinux disabled
State Changed: start
INFO: Start(SRPMS/codemirror-0.91-1.fc13.src.rpm)  Config(fedora-13-i386)
State Changed: lock buildroot
State Changed: clean
INFO: chroot (/var/lib/mock/fedora-13-i386) unlocked and deleted
State Changed: init
State Changed: lock buildroot
Mock Version: 1.1.6
INFO: Mock Version: 1.1.6
INFO: enabled root cache
INFO: root cache aged out! cache will be rebuilt
INFO: enabled yum cache
State Changed: cleaning yum metadata
INFO: enabled ccache
State Changed: running yum
State Changed: creating cache
State Changed: setup
State Changed: build
INFO: Done(SRPMS/codemirror-0.91-1.fc13.src.rpm) Config(default) 1 minutes 57 seconds
INFO: Results and/or logs in: /var/lib/mock/fedora-13-i386/result

Ran successful koji scratch builds
$ koji build --scratch dist-f13 codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f13-updates-candidate codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f14 codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f14-gobject codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f14-updates-candidate codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f15 codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f15-ghc codemirror-0.91-1.fc13.src.rpm
$ koji build --scratch dist-f15-kde codemirror-0.91-1.fc13.src.rpm

A JavaScript library similar in packaging was approved and has
existed in Fedora for quite a while. The name of the package is MochiKit which
can be seen here http://pkgs.fedoraproject.org/gitweb/?p=MochiKit.git;a=tree.
Here is a link to the approved review
Comment 1 Michael Schwendt 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.


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
> $


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.
Comment 2 Jason Tibbitts 2012-04-24 19:50:23 EDT
Hmm, no response to the above commentary in 16 months.  I'm just going to close this.