Bug 543383

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

Spec URL: http://sagarun.fedorapeople.org/SPECS/emacs-irsim-mode.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/emacs-irsim-mode-0.1-2.fc12.src.rpm

Koji builds EPEL,F12:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1843275
http://koji.fedoraproject.org/koji/taskinfo?taskID=1843279

Everything addressed except #004

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

Ok, then I'll start the review

#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}

Spec URL:http://sagarun.fedorapeople.org/SPECS/emacs-irsim-mode.spec
SRPM URL:http://sagarun.fedorapeople.org/SRPMS/emacs-irsim-mode-0.1-3.fc12.src.rpm

Koji builds EPEL,F12 :
http://koji.fedoraproject.org/koji/taskinfo?taskID=1843821
http://koji.fedoraproject.org/koji/taskinfo?taskID=1843823

Issues #001 #002 #003 #004 #005 fixed.

s/if indentation./of indentation/g

I have tested this package on Fedora 12 with Emacs 23.1-12 and it works fine!

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

#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

Spec URL: http://sagarun.fedorapeople.org/SPECS/emacs-irsim-mode.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/emacs-irsim-mode-0.1-4.fc12.src.rpm

Issues #007 and #008 addressed.

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

(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

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.

#009 #010 Fixed.

Spec URL:http://sagarun.fedorapeople.org/SPECS/emacs-irsim-mode.spec
SRPM URL:http://sagarun.fedorapeople.org/SRPMS/emacs-irsim-mode-0.1-5.fc12.src.rpm

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.

Updated

Spec URL: http://sagarun.fedorapeople.org/SPECS/emacs-irsim-mode.spec

SRPM URL: 
http://sagarun.fedorapeople.org/SRPMS/emacs-irsim-mode-0.1-6.fc12.src.rpm

- 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

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

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

cvs done.

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

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

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

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

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.

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.

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.