Bug 655457 - Review Request: codemirror - In-browser code editing made bearable
Review Request: codemirror - In-browser code editing made bearable
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2010-11-20 21:35 EST by Steven Garcia
Modified: 2012-04-24 19:50 EDT (History)
2 users (show)

See Also:
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: ---


Attachments (Terms of Use)

  None (edit)
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 \
SRPMS/codemirror-0.91-1.fc13.src.rpm
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
https://koji.fedoraproject.org/koji/tasks?owner=fosdevel&state=all
$ koji build --scratch dist-f13 codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613671
$ koji build --scratch dist-f13-updates-candidate codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613676
$ koji build --scratch dist-f14 codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613675
$ koji build --scratch dist-f14-gobject codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613679
$ koji build --scratch dist-f14-updates-candidate codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613672
$ koji build --scratch dist-f15 codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613681
$ koji build --scratch dist-f15-ghc codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613687
$ koji build --scratch dist-f15-kde codemirror-0.91-1.fc13.src.rpm
http://koji.fedoraproject.org/koji/taskinfo?taskID=2613685

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
https://bugzilla.redhat.com/show_bug.cgi?id=176528.
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.

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

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