Bug 429323 - Review Request: emacs-lua - Lua major mode for GNU Emacs and XEmacs
Review Request: emacs-lua - Lua major mode for GNU Emacs and XEmacs
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2008-01-18 12:55 EST by Tim Niemueller
Modified: 2008-01-29 17:26 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-01-29 17:26:06 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
dennis: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Description Tim Niemueller 2008-01-18 12:55:29 EST
Spec URL: http://fedorapeople.org/~timn/emacs-lua/emacs-lua.spec
SRPM URL: http://fedorapeople.org/~timn/emacs-lua/emacs-lua-20071122-1.fc8.src.rpm
Description: This package contains a major mode for editing Lua (http://www.lua.org) files. It contains a patch to make it work for XEmacs. The website of the package can be found at http://lua-mode.luaforge.net/. I have sample packages to try it out at http://fedorapeople.org/~timn/emacs-lua/. There are no xemacs/emacs common files thus there is no emacs-common-lua package.
Comment 1 Jason Tibbitts 2008-01-20 15:06:21 EST
This fails to build for me.  I think you really do need to use the mildly
complicated conditional definitions from the template in the packaging
guidelines or otherwise need to take steps to ensure that the spec is
syntactically correct when the srpm is initially built by the buildsystem:

sh: pkg-config: command not found
error: line 46: Version required: Requires: xemacs >=
Building target platforms: x86_64
Building for target x86_64
EXCEPTION: Command failed. See logs for output.
Comment 2 Tim Niemueller 2008-01-21 04:02:17 EST
Hi Jason. Thanks for the review.

- BR pkgconfig
- Excplicitly BR the minimum versions of emacs-el and xemacs-devel that
  provide the pkgconfig files

This should solve the problem that you had without requiring to use the
conditional parts (which is not necessary since an update in F-7 I think).

I have uploaded the new files again to http://fedorapeople.org/~timn/emacs-lua/.
Please give 'em another try.
Comment 3 Jason Tibbitts 2008-01-21 12:38:44 EST
This fails to build in the same manner. The failure is simply due to the fact
that when the srpm is initially built, there is nothing installed.  All external
calls will fail.  If your spec is not syntactically correct even when those
external calls fail, your package will fail to build.  Look at the error:

sh: pkg-config: command not found
error: line 47: Version required: Requires: xemacs >=

After the initial macro expansion with pkg-config not installed, your package
just has "Requires: xemacs >=" with no version number, which is not acceptable
syntax, so the build fails.

Please do a koji scratch build if you want to see this failure in action for
Comment 4 Tim Niemueller 2008-01-21 20:19:51 EST
Jason, sorry for that. I didn't realize that pkgconfig is not in the default
buildroot and thus not available to fill these variables. I do now use the full
templates. To not waste more of your time a successful scratch build is at
Comment 5 Jason Tibbitts 2008-01-24 19:32:04 EST
I grabbed the src.rpm from koji.  It does build OK; rpmlint says:
  emacs-lua.noarch: W: no-documentation
  emacs-lua-el.noarch: W: no-documentation
  xemacs-lua.noarch: W: no-documentation
  xemacs-lua-el.noarch: W: no-documentation
which are OK; there's just a single .el file in this package which gets
byte-compiled.  Actually this package is pretty much trivial.

One thing you might want to do is use "cp -p" in %build to preserve the
timestamps on the el file.

It looks like the *emacs-lua-el packages do not require the *emacs-lua packages.
   This is a blocker.

* source files match upstream:
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summaries are OK.
* descriptions are OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text not included upstream.
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* rpmlint has acceptable complaints.
X final provides and requires:
   emacs-lua = 20071122-3.fc9
   emacs(bin) >= emacs_version

   emacs-lua-el = 20071122-3.fc9
X  (should require emacs-lua)

   xemacs-lua = 20071122-3.fc9
   xemacs >= 21.5.28

   xemacs-lua-el = 20071122-3.fc9
X  (should require xemacs-lua)

* %check is not present; I did not test this package.
* owns the directories it creates (ignoring dependency errors above)
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* no documentation at all to package.
Comment 6 Tim Niemueller 2008-01-26 10:34:45 EST
I have uploaded new packages to http://fedorapeople.org/~timn/emacs-lua/ that
include the mentioned requires. It also uses "cp -p" to preserve the timestamp.
Comment 7 Jason Tibbitts 2008-01-26 17:12:50 EST
The dependencies look good to me now.

I'm still not seeing the timestamp of the .el files preserved so maybe just
using cp -p wasn't sufficient, but it's not a blocker in any case.

Comment 8 Ville Skyttä 2008-01-26 17:26:08 EST
We (upstream XEmacs folks) are already shipping a version of lua-mode.el that
has been tweaked to work with XEmacs in our (lisp) packages collection, and it
is also included in the xemacs-package-extra (rpm) package in Fedora.  Shipping
another copy for XEmacs does not sound like a good idea to me.
Comment 9 Ville Skyttä 2008-01-26 17:26:41 EST
Comment 10 Jason Tibbitts 2008-01-26 17:38:13 EST
Hmm, I had no clue; it's not exactly easy to figure that out.  Well, Tim, I
guess you can strip the xemacs stuff from this package, then.

Inspection reveals that the version included there is several years older,
though, and seemingly unrelated to the lua-mode in this package.  Someone should
evaluate and possibly consider dropping one or the other.
Comment 11 Ville Skyttä 2008-01-26 17:58:48 EST
Agreed, it could be easier, but the Principles chapter of
http://fedoraproject.org/wiki/Packaging/Emacs pretty has some related info.

But actually, the mode in xemacs-packages-extra and this one seem close to each
other; the one in xemacs-packages-extra is just an ancestor of this one,
imported to the XEmacs packages collection before an "official" upstream for
lua-mode.el existed, and it has got some XEmacs specific tweaks since.  I'll ask
around on XEmacs lists if someone would be willing to synch up our version with
this one.
Comment 12 Tim Niemueller 2008-01-27 07:04:05 EST
Hmm, I didn't notice the Lua support. In fact I just use Emacs and just packaged
it for XEmacs since the templates made it really easy to do. I'll drop the
XEmacs support then. This is good since this will also eliminate the patch,
which was only needed to get it working with XEmacs. Would be great if you could
drop a note in this rr if/when someone syncs up the XEmacs bits. Since no more
copy is needed this should also fix the timestamp issue.

Uploaded new packages to http://fedorapeople.org/~timn/emacs-lua/

Are you going to "re-approve", Jason?
Comment 13 Jason Tibbitts 2008-01-27 14:29:48 EST
You don't need me to re-approve, but I looked over the spec and it seems OK to me.
Comment 14 Tim Niemueller 2008-01-27 18:04:58 EST
New Package CVS Request
Package Name: emacs-lua
Short Description: Lua major mode for GNU Emacs
Owners: timn
Branches: F-7 F-8
InitialCC: timn
Cvsextras Commits: yes
Comment 15 Dennis Gilmore 2008-01-28 11:03:14 EST
You do not need to add yourself to initallCC 

CVS Done
Comment 16 Tim Niemueller 2008-01-29 17:26:06 EST
Pushed to stable. Should soon appear at a repo near you. Thanks for the thorough
review, Jason.

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