Bug 543383 - Review Request: emacs-irsim-mode - Irsim mode for emacs
Review Request: emacs-irsim-mode - Irsim mode for emacs
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Chitlesh GOORAH
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-02 05:23 EST by Arun S A G
Modified: 2010-05-18 22:41 EDT (History)
3 users (show)

See Also:
Fixed In Version: emacs-irsim-mode-0.1-6.el5
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-11 13:19:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
chitlesh: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Arun S A G 2009-12-02 05:23:47 EST
Spec URL: http://sagarun.fedorapeople.org/SPECS/emacs-irsim-mode.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/emacs-irsim-mode-0.1-1.fc12.src.rpm
Description: 
This package is provides Emacs mode for editing IRSIM netlists. IRSIM is a switch-level simulator for digital logic circuits. 


koji builds EPEL, F12,F11: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=1842928
http://koji.fedoraproject.org/koji/taskinfo?taskID=1842936
http://koji.fedoraproject.org/koji/taskinfo?taskID=1842950
Comment 1 Shakthi Kannan 2009-12-02 06:24:29 EST
#001: Use %{__rm} instead of rm.
      Use %{__mkdir} instead of mkdir.

#002: Use %{__install} instead of having to define % global INSTALL.

#003: In %prep, %build section, you are doing the same (cd) twice. Not required.

#004: There is no README file?

#005: These are redundant!

  %global pkg emacs-irsim-mode
  %global common_name irsim-mode
  %global pkgname Emacs-irsim-mode
  %global pkgdir %{buildroot}%{emacs_lispdir}/irsim-mode/
  %global ver 0.1
  %global rel 1

#006: Instead of 'ver', 'rel', you can call them something else? Avoid short-hand as much as possible. .spec needs to be readable.

#007:

  "This package contains the elisp source files for 
  Emacs-irsim-mode under GNU Emacs.
  You do not need to install this package 
  to run Emacs-irsim-mode. 
  Install the emacs-irsim-mode package 
  to use %{pkgname}  with GNU Emacs."

The grammar in the above is incorrect. You just copied the text? :) Why do you need to give instructions on packaging it in the description? Just keep the following in the description:

  This package contains elisp source files for Emacs-irsim-mode for use with
  GNU Emacs.
Comment 3 Chitlesh GOORAH 2009-12-02 08:03:38 EST
Arun, update the spec and srpm as Shakthi recommends. Afterwards, I'll complete the review. If you have based your spec file on mine, please include its changelog as well.
https://fedorahosted.org/fedora-electronic-lab/attachment/ticket/50/emacs-irsim-mode.spec
Comment 4 Chitlesh GOORAH 2009-12-02 08:04:13 EST
Ok, then I'll start the review
Comment 5 Chitlesh GOORAH 2009-12-02 09:20:35 EST
#001: Description
Use the following description instead :
32 	IRSIM is a switch-level simulator for digital logic circuits.
33 	This is an Emacs mode for editing IRSIM netlists. It provides
34 	syntax highlighting and an extremely pleasant method if indentation.

#002: Start file

It should better have the following since I think it should be autoload
49 	(autoload 'irsim-mode "irsim-mode" nil t)
50 	(setq auto-mode-alist
51 	      (cons '("\\.sim$" . irsim-mode) auto-mode-alist))


#003: compile it with

emacs -batch -f batch-byte-compile %{pkg}.el

#004 Directory ownership and duplicates

Verify why you are having double directory ownership in the %files section for
%{emacs_startdir}

#005 Keep the spec file simple

replace  cd %{_builddir}/%{name}-%{version} by cd %{name}-%{version}
Comment 7 Shakthi Kannan 2009-12-02 13:20:02 EST
s/if indentation./of indentation/g

I have tested this package on Fedora 12 with Emacs 23.1-12 and it works fine!
Comment 8 Chitlesh GOORAH 2009-12-02 14:13:18 EST
#006 You don't need to create scratch for everytime you post your SRPM. The main idea of doing a scratch built is to verify whether you have all the buildrequires on your spec file. One scratch build is enough.

#007 I would advice to autoload irsim-mode for 
 - "*.cmd" 
 - "*.simout"
 - "*.flt"
 - "*.sim"
files as well.
Comment 9 Chitlesh GOORAH 2009-12-02 14:18:25 EST
#008 Preserve timestamps
Your
cp %{SOURCE0} %{_builddir}/%{name}-%{version}
cp %{SOURCE1} %{_builddir}/%{name}-%{version}
should be 
cp -p %{SOURCE0} %{_builddir}/%{name}-%{version}
cp -p %{SOURCE1} %{_builddir}/%{name}-%{version}


so should be your
%{__install} -m
to
%{__install} -pm
Comment 11 Chitlesh GOORAH 2009-12-06 13:06:46 EST
#009 Installation fails on EL-5,
%global emacs_version 23.1.1
should be
%global emacs_version 21.0

(Also update your other spec files accordingly)

[root@noname ~]# rpm -Uvh /home/chitlesh/Desktop/emacs-irsim-mode-0.1-4.el5.noarch.rpm 
error: Failed dependencies:
        emacs(bin) >= 23.1.1 is needed by emacs-irsim-mode-0.1-4.el5.noarch
[root@noname ~]# rpm -q emacs
emacs-21.4-20.el5

#010 my mistake on #007 : "*.simout" should be "*.out". Why *.out ? because during stuck-at fault simulation, irsim outputs a default fsim.out file.



There are two features in this irsim-mode package:
 * indentation
 * highlight

My test on fedora-12 works as expected. Please update the package accordingly and final review.
Comment 12 Arun S A G 2009-12-06 21:29:48 EST
(In reply to comment #11)
> #009 Installation fails on EL-5,
> %global emacs_version 23.1.1
> should be
> %global emacs_version 21.0

Hi, according to emacs packaging guidelines draft it should be 21.1 ? http://fedoraproject.org/wiki/Packaging/Emacs#Packaging_of_add-ons_for_GNU_Emacs_and_XEmacs
Comment 13 Chitlesh GOORAH 2009-12-07 00:10:58 EST
Actually that's a template for fedora but not for centos-5. Since centos-5 has emacs-21.4-20.el5, we've got to use %global emacs_version 21.1 instead.
Comment 15 Chitlesh GOORAH 2009-12-07 15:13:19 EST
The package looks ok. Only one glitch left it's the different RPM version on fedora and centos.

You will need to replace Requires: emacs(bin) by Requires: emacs.

The package as it is is ok for Fedora repos. Update this package for the last time and we are good to go.
Comment 17 Chitlesh GOORAH 2009-12-08 04:37:36 EST
- MUST: The package is named according to the Package Naming Guidelines.
- MUST: The spec file name matches the base package emacs-%{name}
- MUST: The package meets the Packaging Guidelines.
- MUST: The package is licensed with an open-source compatible license and meet other legal requirements as defined in the legal section of Packaging Guidelines.
- MUST: The License field in the package spec file matches the actual license.
- MUST: The spec file must be written in American English.
- MUST: The spec file for the package is be legible. 
- MUST: The sources used to build the package must matches the upstream source,
as provided in the spec URL.
- MUST: The package successfully compiles and builds into binary rpms on at
least i686.
- MUST: All build dependencies is listed in BuildRequires.
- MUST: The spec file handles locales properly. Emacs-irsim-mode does not have any.
- MUST: If the package does not contain shared library files located in the
dynamic linker's default paths. Emacs-irsim-mode does not have any.
- MUST: the package is not designed to be relocatable
- MUST: the package owns all directories that it creates.
- MUST: the package does not contain any duplicate files in the %files listing.
- MUST: Permissions on files are set properly.
- MUST: The package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
- MUST: The package consistently uses macros, as described in the macros section
of Packaging Guidelines.
- MUST: The package contains code, or permissable content. This is described in
detail in the code vs. content section of Packaging Guidelines.
- MUST: There are no Large documentation files. Emacs-irsim-mode does not have any.
- MUST: %doc does not affect the runtime of the application. To summarize: If it
is in %doc, the program must run properly if it is not present.
- MUST: There are no Header files or static libraries 
- MUST: The package does not contain library files with a suffix 
- MUST: Package does NOT contain any .la libtool archives
- MUST: Package containing GUI applications includes a %{name}.desktop file, and
that file must be properly installed with desktop-file-install in the %install
section.
- MUST: Package does not own files or directories already owned by other packages. 

SHOULD Items:

 - SHOULD: mock builds succcessfully in i686.
 - SHOULD: The reviewer tested that the package functions as described. A
package should not segfault instead of running, for example.
 - SHOULD: No scriptlets were used, those scriptlets must be sane. 

APPROVED
Comment 18 Chitlesh GOORAH 2009-12-08 04:41:33 EST
Request CVS branches as explained on here and and me on CC: I just want to monitor your first few packagers while you are doing your RPM baby steps :D

https://fedorahosted.org/fedora-electronic-lab/ticket/19#comment:7
Comment 19 Arun S A G 2009-12-08 05:01:05 EST
New Package CVS Request
=======================
Package Name: emacs-irsim-mode
Short Description: Irsim mode for emacs
Owners: sagarun chitlesh
Branches: F-11 F-12 EL-5
Comment 20 Kevin Fenzi 2009-12-09 12:52:41 EST
cvs done.
Comment 21 Fedora Update System 2009-12-09 14:13:25 EST
emacs-irsim-mode-0.1-6.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/emacs-irsim-mode-0.1-6.fc12
Comment 22 Fedora Update System 2009-12-09 14:13:41 EST
emacs-irsim-mode-0.1-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/emacs-irsim-mode-0.1-6.fc11
Comment 23 Fedora Update System 2009-12-09 14:13:52 EST
emacs-irsim-mode-0.1-6.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/emacs-irsim-mode-0.1-6.el5
Comment 24 Fedora Update System 2009-12-09 23:02:55 EST
emacs-irsim-mode-0.1-6.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update emacs-irsim-mode'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/EL-5/FEDORA-EPEL-2009-1001
Comment 25 Fedora Update System 2009-12-11 13:19:07 EST
emacs-irsim-mode-0.1-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 26 Fedora Update System 2009-12-11 13:30:07 EST
emacs-irsim-mode-0.1-6.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 27 Fedora Update System 2010-05-18 22:41:22 EDT
emacs-irsim-mode-0.1-6.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

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