Bug 494647 - Review Request: emacs-mmm - Emacs minor mode allowing different major modes in the same file
Summary: Review Request: emacs-mmm - Emacs minor mode allowing different major modes i...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jerry James
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-04-07 17:37 UTC by Alan Dunn
Modified: 2009-04-13 19:44 UTC (History)
3 users (show)

Fixed In Version: 0.4.8-1.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-13 19:43:50 UTC
Type: ---
Embargoed:
loganjerry: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to quiet the Emacs byte compiler (2.11 KB, patch)
2009-04-08 17:31 UTC, Jerry James
no flags Details | Diff

Description Alan Dunn 2009-04-07 17:37:27 UTC
Spec URL: http://www.phy.duke.edu/~amd34/emacs-mmm/emacs-mmm.spec
SRPM URL: http://www.phy.duke.edu/~amd34/emacs-mmm/emacs-mmm-0.4.8-1.fc10.src.rpm
Description:

MMM Mode is an emacs add-on package providing a minor mode that allows
Multiple Major Modes to coexist in one buffer. It is particularly
well-suited to editing embedded code or code that generates other
code, such as Mason or Embperl server-side Perl code, or HTML output
by CGI scripts.

MMM Mode defines a general syntax--the "submode class"--for telling it
how one major mode should be embedded in another. At present, the list
of supplied submode classes is rather limited, but that will hopefully
change soon. Contributions are always welcome; writing a submode class
can be a good introduction to Emacs Lisp, and is usually a simple
excercise for those already proficient. MMM Mode was originally
designed to work in Emacs 19 and 20 and XEmacs 20 and 21.

This has been created as an emacs-only package as it already exists
for XEmacs in xemacs-packages-extra.

Testing: I have installed this on my own machine and it appears to work. It build in koji (f9, f10, though oddly not in f11 I just found out - perhaps something weird with Koji at the moment for rawhide? :
f9: http://koji.fedoraproject.org/koji/taskinfo?taskID=1283407
f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=1283409
f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=1283419
)

rpmlint output:
[adunn@localhost rpmbuild]$ rpmlint SPECS/emacs-mmm.spec ~/rpmbuild/RPMS/noarch/emacs-mmm-* ~/rpmbuild/SRPMS/emacs-mmm-0.4.8-1.fc10.src.rpm
emacs-mmm-el.noarch: W: no-documentation
3 packages and 1 specfiles checked; 0 errors, 1 warnings.

and I believe that message is acceptable.

Comment 1 Jerry James 2009-04-08 17:09:57 UTC
I have a couple of questions about the spec file:

1) Is there some reason to avoid the %configure macro in the spec file?  (And "s/Not/Note" in the comment above the configure invocation if so.)

2) Shouldn't this package "Requires: emacs(bin)"?

You noted a problem with "make mmm.pdf".  The problem is that the texinfo.tex in the mmm-mode distribution is being used, and it is too old (it's from 1999!).  Just rm it, and then the system texinfo.tex gets used, and you get a good build.

Some of the Emacs Lisp warnings have me concerned, especially the notes that regexp-opt is being called with 3 arguments but only accepts 1 or 2.  This is almost certain to lead to broken compiled code.  I'll make a patch for you.  Hang on....

Comment 2 Jerry James 2009-04-08 17:31:04 UTC
Created attachment 338763 [details]
Patch to quiet the Emacs byte compiler

Oops, my mistake, I managed to compile by hand with XEmacs instead of Emacs while trying out the mmm.pdf fix. :-(  Nevertheless, the attached patch, while not necessary, does quiet the byte compiler down a bit.  Take it or leave it as you see fit.

Comment 3 Alan Dunn 2009-04-08 17:39:26 UTC
(In reply to comment #1)
> I have a couple of questions about the spec file:
> 
> 1) Is there some reason to avoid the %configure macro in the spec file?  (And
> "s/Not/Note" in the comment above the configure invocation if so.)

I actually would've, but I was told by another person ("pre-review"?) that it was wise to avoid %configure. I was trying to remember the reason for this, but I noticed that I have done this in many of my spec files, so I did it here too, adding exactly the options I thought was necessary. If I can't remember the reason, I'll change it back. (The "Not" was supposed to be a "Not" - it doesn't really matter here, but rpmlint complains otherwise. I suppose this would only occur if you didn't use %configure)

> 2) Shouldn't this package "Requires: emacs(bin)"?

Yes, it should, sorry.

> You noted a problem with "make mmm.pdf".  The problem is that the texinfo.tex
> in the mmm-mode distribution is being used, and it is too old (it's from
> 1999!).  Just rm it, and then the system texinfo.tex gets used, and you get a
> good build.

Ah - great, I'll do that.

> Some of the Emacs Lisp warnings have me concerned, especially the notes that
> regexp-opt is being called with 3 arguments but only accepts 1 or 2.  This is
> almost certain to lead to broken compiled code.  I'll make a patch for you. 
> Hang on....

Comment 4 Alan Dunn 2009-04-08 18:04:21 UTC
Ok, I the changes (switched back -> %configure, required emacs(bin), added patch) you suggested, and the new files are in the same locations as the old were.

(In reply to comment #3)
> (In reply to comment #1)
> > I have a couple of questions about the spec file:
> > 
> > 1) Is there some reason to avoid the %configure macro in the spec file?  (And
> > "s/Not/Note" in the comment above the configure invocation if so.)
> 
> I actually would've, but I was told by another person ("pre-review"?) that it
> was wise to avoid %configure. I was trying to remember the reason for this, but
> I noticed that I have done this in many of my spec files, so I did it here too,
> adding exactly the options I thought was necessary. If I can't remember the
> reason, I'll change it back. (The "Not" was supposed to be a "Not" - it doesn't
> really matter here, but rpmlint complains otherwise. I suppose this would only
> occur if you didn't use %configure)
> 
> > 2) Shouldn't this package "Requires: emacs(bin)"?
> 
> Yes, it should, sorry.
> 
> > You noted a problem with "make mmm.pdf".  The problem is that the texinfo.tex
> > in the mmm-mode distribution is being used, and it is too old (it's from
> > 1999!).  Just rm it, and then the system texinfo.tex gets used, and you get a
> > good build.
> 
> Ah - great, I'll do that.
> 
> > Some of the Emacs Lisp warnings have me concerned, especially the notes that
> > regexp-opt is being called with 3 arguments but only accepts 1 or 2.  This is
> > almost certain to lead to broken compiled code.  I'll make a patch for you. 
> > Hang on....

Comment 5 Jerry James 2009-04-08 19:42:58 UTC
Okay, now it looks like this bit of %install ...

# Fix an issue that occurs in the info files (not quite sure where in
# the process, so doing it here):

mv %{buildroot}%{_infodir}/mmm.info-2.gz \
   %{buildroot}%{_infodir}/mmm.info-2 && \
   gzip %{buildroot}%{_infodir}/mmm.info-2

... is no longer necessary.  At least, it wasn't necessary for me.

MUST items:
- rpmlint output:
emacs-mmm-el.noarch: W: no-documentation
2 packages and 1 specfiles checked; 0 errors, 1 warnings.
- package name
- spec file name
- packaging guidelines
- Fedora approved license
- license field matches actual license
- license file in %doc
- spec file in American English
- spec file is legible
- sources match upstream
- builds on at least one primary arch
- use of ExcludeArch (N/A)
- build dependencies in BuildRequires
- locale handling (N/A)
- call ldconfig (N/A)
- relocatable package (N/A)
- own all created directories
- no duplicates in %files
- proper permissions
- %clean section
- consistent use of macros
- code or permissible content
- large documentation in -doc (N/A)
- nothing in %doc needed at runtime
- header files in -devel (N/A)
- static libraries in -static (N/A)
- Requires pkgconfig (N/A)
- .so files in -devel (N/A)
- -devel requires main package (N/A)
- no libtool archives (N/A)
- desktop file (N/A)
- do not own previously owned dirs
- clean at start of %install
- filenames are valid UTF-8

SHOULD items:
- ask upstream to include license file (N/A)
- supply available translations (N/A)
? package builds in mock (didn't check)
? builds on all supported arches (didn't check)
- package functions as described
- sane scriptlets
- subpackages require main package
- pkgconfig file in -devel (N/A)
- file vs. package dependencies

Looks good.  APPROVED.

Comment 6 Alan Dunn 2009-04-09 14:10:10 UTC
(In reply to comment #5)
> Okay, now it looks like this bit of %install ...
> 
> # Fix an issue that occurs in the info files (not quite sure where in
> # the process, so doing it here):
> 
> mv %{buildroot}%{_infodir}/mmm.info-2.gz \
>    %{buildroot}%{_infodir}/mmm.info-2 && \
>    gzip %{buildroot}%{_infodir}/mmm.info-2
> 
> ... is no longer necessary.  At least, it wasn't necessary for me.

Really? Any idea that is... the problem I found for my system was that mmm.info-2.gz was not actually a zipped archive. Thus, if you run info and navigate to a page that requires mmm.info-2, info actually segfaults. It still seems necessary for me: the result in the RPM is a (once zipped) info file. If this problem didn't occur on your system, then this "fix" would actually break things, so I want to know. (I suppose it might be safer to do file type detection and zip it if it isn't a zip file already.)

Comment 7 Alan Dunn 2009-04-09 14:18:11 UTC
Hmm, yes, it now seems unnecessary on my system too - the command isn't actually doing anything (I thought any error would've caused rpmbuild to stop, but on closer inspection the command was having an error - couldn't find the zipped file - and not doing anything...)

(In reply to comment #6)
> (In reply to comment #5)
> > Okay, now it looks like this bit of %install ...
> > 
> > # Fix an issue that occurs in the info files (not quite sure where in
> > # the process, so doing it here):
> > 
> > mv %{buildroot}%{_infodir}/mmm.info-2.gz \
> >    %{buildroot}%{_infodir}/mmm.info-2 && \
> >    gzip %{buildroot}%{_infodir}/mmm.info-2
> > 
> > ... is no longer necessary.  At least, it wasn't necessary for me.
> 
> Really? Any idea that is... the problem I found for my system was that
> mmm.info-2.gz was not actually a zipped archive. Thus, if you run info and
> navigate to a page that requires mmm.info-2, info actually segfaults. It still
> seems necessary for me: the result in the RPM is a (once zipped) info file. If
> this problem didn't occur on your system, then this "fix" would actually break
> things, so I want to know. (I suppose it might be safer to do file type
> detection and zip it if it isn't a zip file already.)

Comment 8 Alan Dunn 2009-04-09 14:21:29 UTC
New Package CVS Request
=======================
Package Name: emacs-mmm
Short Description: Emacs minor mode allowing different major modes in the same file
Owners: amdunn
Branches: F-9 F-10 F-11

Comment 9 Kevin Fenzi 2009-04-10 00:15:40 UTC
cvs done.

Comment 10 Fedora Update System 2009-04-13 11:47:36 UTC
emacs-mmm-0.4.8-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/emacs-mmm-0.4.8-1.fc9

Comment 11 Fedora Update System 2009-04-13 11:47:42 UTC
emacs-mmm-0.4.8-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/emacs-mmm-0.4.8-1.fc10

Comment 12 Fedora Update System 2009-04-13 19:43:45 UTC
emacs-mmm-0.4.8-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 13 Fedora Update System 2009-04-13 19:44:00 UTC
emacs-mmm-0.4.8-1.fc10 has been pushed to the Fedora 10 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.