Bug 544745 - Review Request: emacs-spice-mode - SPICE Mode for GNU Emacs
Summary: Review Request: emacs-spice-mode - SPICE Mode for GNU Emacs
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chitlesh GOORAH
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-12-06 10:56 UTC by Arun S A G
Modified: 2010-05-09 08:38 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-05-09 08:38:07 UTC
Type: ---
Embargoed:
chitlesh: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Fix free variables and backquote (3.39 KB, patch)
2009-12-06 16:56 UTC, Shakthi Kannan
no flags Details | Diff
Combined shakthi's patch with mine (3.95 KB, patch)
2009-12-07 04:20 UTC, Arun S A G
no flags Details | Diff
updated init file with default setting for simulator and viewer (980 bytes, text/plain)
2009-12-12 21:44 UTC, Chitlesh GOORAH
no flags Details
.el update with minor fixes (302.38 KB, text/plain)
2009-12-12 21:46 UTC, Chitlesh GOORAH
no flags Details

Comment 1 Shakthi Kannan 2009-12-06 15:08:02 UTC
Avoid shorthand in spice-mode-init.el

  s/eldo outpt/eldo output/

I have tested this package and it looks ok.

Comment 2 Chitlesh GOORAH 2009-12-06 15:13:53 UTC
Can you have a look and fix if possible the warnings during the build please?
Also add .spice and .spc in the init.el file as they are the output format for magic and xcircuit.

Comment 3 Shakthi Kannan 2009-12-06 16:56:20 UTC
Created attachment 376474 [details]
Fix free variables and backquote

This fixes the free variables and the backquote warning.

Comment 4 Chitlesh GOORAH 2009-12-06 16:57:23 UTC
#001: missing requires : gwave and ngspice

#002: obselete spice3 in favour of ngspice

Please patch the sources so that on the spice menu, we can see ngspice as one of the simulator and ensure that it is the default one since we don't ship any of the listed spice simulators.

"spice3 -b" should be "ngspice -b"


You can use a testcase from FEL's git repo e.g ota.sp
http://git.fedorahosted.org/git/fedora-electronic-lab.git?p=fedora-electronic-lab.git;a=blob_plain;f=testsuite/ngspice/ota.sp

Comment 5 Chitlesh GOORAH 2009-12-06 19:36:34 UTC
for some reason, when I simulate ota.sp with ngspice on emacs, emacs gets freezed and I have to kill it.

However with gnucap it works as expected without having to kill emacs.

can you reproduce it ? if you encounter this issue, then I have to notify ngspice's upstream.

Comment 6 Arun S A G 2009-12-07 04:20:17 UTC
Created attachment 376575 [details]
Combined shakthi's patch with mine

Combined Shakthi's patch and made changes to make ngspice as default simulator. Please test if simulation works.

Comment 7 Chitlesh GOORAH 2009-12-07 07:16:27 UTC
Your patch is working as expected on F-12. I'm currently testing it under F-12 and will test on centos-5 soon.

The following are some improvements that we need to make and ensure. The spice-mode seems to be meant for proprietary spice simulators and is very old. Thereby, we need to tune it as appropriate

#003: in the spice simulators list, obsolete  
"Nutmeg" "nutmeg"
by
"Nutmeg" "ngnutmeg"

ngnutmeg is provided by ngspice package.

#004: in the spice simulators list, please add gnucap. gnucap has a -b switch as ngspice. So you can safely copy ngspice section and paste it for gnucap. (create a separate patch for this). In the meantime, I'll contact Al Davis to see whether he already has some fine tunings for emacs.

#005: ngspice still freezes emacs.

#006: In the init file, you have different file extensions and for each you have a separate auto-mode-alist entry e.g
(setq auto-mode-alist
      (cons '("\\.sim$" . irsim-mode) auto-mode-alist))

I'm not an emacs expect: Do several entries affect the load time of emacs ?

#007: Set gnucap as default (since ngspice is freezing emacs). Remove ngspice from the Requires and add gnucap as BR

#008: Warning for emacs >= 22

%if 0%{?fedora}
sed -i "s|process-kill-without-query|process-query-on-exit-flag|" %{pkg}.el
%endif

#009: Can you set gnucap as a default simulator and gwave as a default viewer please ? Possibly in the init file

#010: Debatable:
If one opens a spice netlist with emacs-spice-mode, one will see the names of proprietary spice standards: Hspice,Eldo... as my attached screenshot demonstrates.

I think we need to replace that with "Gnucap/Ngspice" if not it will look as if we are doing publicity for those vendors.

Comment 8 Chitlesh GOORAH 2009-12-12 21:44:52 UTC
Created attachment 377925 [details]
updated init file with default setting for simulator and viewer

Comment 9 Chitlesh GOORAH 2009-12-12 21:46:01 UTC
Created attachment 377926 [details]
.el update with minor fixes

Comment 10 Chitlesh GOORAH 2009-12-12 21:50:41 UTC
The attached files are an updated init file and updated .el file which fixes the above items.

I would advice you that next time when you create patches don't merge patches from another one. It is just a matter of respect someone's time spent to create a patch. In this case it's Shakthi's time. I won't block the package because of this. However I feel that Shakthi's contribution deserved to be recognised. :)

Please update the package for final review.

Comment 11 Arun S A G 2009-12-13 03:18:27 UTC
(In reply to comment #10)
> I would advice you that next time when you create patches don't merge patches
> from another one. It is just a matter of respect someone's time spent to create
> a patch. In this case it's Shakthi's time. I won't block the package because of
> this. However I feel that Shakthi's contribution deserved to be recognised. :)

Agreed :-) . I made a patch out of the updated spice-mode.el file and before patch0 line of SPEC file, i gave credits (as comment) to the authors. is that ok?

(In reply to comment #7)
> #007: Set gnucap as default (since ngspice is freezing emacs). Remove ngspice
> from the Requires and add gnucap as BR

I doubt this, do i have to add gnucap as Build requires? what is BR ? gnucap is  needed to build the package? May be you meant Requires ?

Comment 12 Chitlesh GOORAH 2009-12-13 10:50:04 UTC
Yes, add gnucap and gwave as requires.

BR: is the shortform to _say_ Build Requires (Buildrequires:)

The gnucap is not used during the building of the rpm hence no need to add as a BR.

Comment 14 Chitlesh GOORAH 2009-12-13 16:32:35 UTC
- 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-spice-mode does not have
any.
- MUST: If the package does not contain shared library files located in the
dynamic linker's default paths. Emacs-spice-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-spice-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 successfully 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 15 Arun S A G 2009-12-13 16:46:27 UTC
New Package CVS Request
=======================
Package Name: emacs-spice-mode
Short Description: SPICE Mode for GNU Emacs
Owners: sagarun chitlesh
Branches: F-11 F-12 EL-5

Comment 16 Kevin Fenzi 2009-12-14 17:27:08 UTC
cvs done.

Comment 18 Chitlesh GOORAH 2009-12-16 12:11:28 UTC
The review is complete.

use ./common/cvs-import.sh to update your package.


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