Bug 480546

Created attachment 329303 [details]
Add an rpm macros file to the package

Some questions/observations:

Because there's no Epoch in the xemacs package, I suppose it would end up as literal '%{epoch}' in the macros file which I guess would cause problems.  Without knowing how is intended to be used it's a bit hard to suggest a fix, but I suppose writing it as %{?epoch} could be one.

_xemacs_sitelispdir should really be _xemacs_sitepkglispdir - in XEmacs site-packages and site-lisp are two different things.  It's already like this in the *.pc file.

The byte compilation command seems to be using some undocumented options (there's only one prefix hyphen in the documented -no-init-file and -no-site-file).  Maybe "/usr/bin/xemacs -batch -vanilla -f batch-byte-compile" would be better.  OTOH I'm not sure if -vanilla (implied -no-early-packages) is a bit too much, if it is, it can be replaced with "-q -no-site-file" to make the command equivalent to your original suggestion.

Wouldn't the macros file be better placed in the -devel package?  They're not needed in normal non-development scenarios, right?  Also, grabbing the values from pkg-config at runtime could be arguably cleaner, but then again I suppose if/when this stuff is in we could consider dropping the *.pc file altogether, right?

There's a s/xemacs/emacs/ typo in the changelog entry.

Hi Ville

(In reply to comment #2)
> Some questions/observations:
> 
> Because there's no Epoch in the xemacs package, I suppose it would end up as
> literal '%{epoch}' in the macros file which I guess would cause problems. 
> Without knowing how is intended to be used it's a bit hard to suggest a fix,
> but I suppose writing it as %{?epoch} could be one.
> 

Yes, it's largely superfluous presently for xemacs, but emacs is already on epoch 1 and so it may be necessary to use the epoch in a Requires for an add-on package for Emacs (i.e. Requires: emacs-common >= %{_emacs_epoch}-%{_emacs_version}). I haven't actually explored that yet and was mainly adding it for future use more than anything. In the revised patch I've changed it to ${?epoch}as you suggest. Or we could just remove that line.

> _xemacs_sitelispdir should really be _xemacs_sitepkglispdir - in XEmacs
> site-packages and site-lisp are two different things.  It's already like this
> in the *.pc file.

Hm. But surely add-on package lisp files would only ever go in /usr/share/xemacs/site-packages/lisp ? There doesn't seem to be a /usr/share/xemacs/site-lisp so I'm not sure I see a need for the distinction of adding the "pkg" to the macro.

The current macro name is consistent with the quivalent emacs macro. I don't have a strong feeling about this though.

Presumably in the future, we may want to add macros like
_xemacs_sitepkgetc
_xemacs_sitepkgman
_xemacs_sitepkgpkginfo

Actually, writing that last one, I really wonder what the extra "pkg" really gains us :)

Also, for consistency, if we have the pkg, should we also rename _xemacs_sitestartdir to _xemacs_sitepkgstartdir ?

Maybe I am missing something?
 
> The byte compilation command seems to be using some undocumented options
> (there's only one prefix hyphen in the documented -no-init-file and
> -no-site-file).  Maybe "/usr/bin/xemacs -batch -vanilla -f batch-byte-compile"
> would be better.  OTOH I'm not sure if -vanilla (implied -no-early-packages) is
> a bit too much, if it is, it can be replaced with "-q -no-site-file" to make
> the command equivalent to your original suggestion.
> 

OK, have set this to:

 /usr/bin/xemacs -q -no-site-file -batch -f batch-byte-compile

since I don't imagine that -no-early-packages is ever the right thing to use for add-on packages?

> Wouldn't the macros file be better placed in the -devel package?  They're not
> needed in normal non-development scenarios, right?  Also, grabbing the values
> from pkg-config at runtime could be arguably cleaner, but then again I suppose
> if/when this stuff is in we could consider dropping the *.pc file altogether,
> right?
> 

I didn't put it in the devel package because:

a) the devel package seems more targetted to people hacking on xemacs internals 9since it's mostly header files)

b) For consistency with emacs which doesn't have a devel package. [aside: with emacs the pc file is in the emacs-el subpackage, which in hindsight I think was a mistake.]

c) It's more consistent with other packages - eg. qt4 contains /etc/rpm/macros.qt4, rather than qt4-devel.

d) It'll make the guidelines simpler as the BuildRequires will just be xemacs, rather than pulling in all of xemacs-devel just for one file.


Regarding the pkgconfig file - yeah, longer term I'd like to see that dropped, but I think we need to have both the macros.[x]emacs and the pc file co-existing for a while to avoid package breakage.

> There's a s/xemacs/emacs/ typo in the changelog entry.

OK will fix, sorry.

Will attach a revised patch.

Created attachment 329310 [details]
Add macros file

Still unresolved in this version: whether to add "pkg" to the macros - I don't have strong feelings either way, but would like to understand slightly better the distinction.

(In reply to comment #3)
> (i.e. Requires: emacs-common >= %{_emacs_epoch}-%{_emacs_version})

I think this will start to look somewhat clumsy when you take the lack of Epoch into account; for a xemacs add-on package that would need to be something like

Requires: xemacs-common >= %{?xemacs_epoch:%{xemacs_epoch}:}%{xemacs_version}

I can't think of a case where the epoch would be useful alone, so I'd suggest using something like

%%_xemacs_evr %{?epoch:%{epoch}:}%{version}-%{release}

...or just *_ev if you don't see value for the release bit in it.  And keep %_xemacs_version as it is now if it's required for something.  I doubt that it is too useful because it shouldn't be used without epoch considerations in rpm package dependencies, and it doesn't occur in any versioned files/dirs either (for xemacs rpm version 21.5.28 the versioned files in it have 21.5-b28).

> Presumably in the future, we may want to add macros like
> _xemacs_sitepkgetc
> _xemacs_sitepkgman
> _xemacs_sitepkgpkginfo

If that's so, then I'd suggest just defining _xemacs_sitepkgdir, pointing it to [...]/site-packages, and forgetting about the rest (== expressing them like %{_xemacs_sitepkgdir}/lisp, %{_xemacs_sitepkgdir}/man etc in specfiles).  I don't think it's possible to configure xemacs so that this assumption about the dir structure it uses for this stuff wouldn't hold.  Having a macro for all of them would just add noise and decrease spec file readability.

> since I don't imagine that -no-early-packages is ever the right thing to use
> for add-on packages?

Well, the cleaner the build path when compiling the better (as long as it has everything necessary), but this would probably be overkill and result in need to add quite a few "-l foo" options to the compilation command almost every time.  I suppose the form in your revised patch is fine.

> a) the devel package seems more targetted to people hacking on xemacs internals
> 9since it's mostly header files)
> 
> b) For consistency with emacs which doesn't have a devel package. [aside: with
> emacs the pc file is in the emacs-el subpackage, which in hindsight I think
> was a mistake.]

Yeah, kind of.  But on the other hand I'm having some doubts whether the -el packages make any sense in the first place.  They shouldn't be normally needed at runtime at all, but rather correspond to -debuginfo.  Or -devel, depending on the POV (and in -devel case I think the macros and *.pc would fit in it just fine).  Ditto for xemacs, perhaps -el should be folded into -devel.  Thoughts?

> c) It's more consistent with other packages - eg. qt4 contains
> /etc/rpm/macros.qt4, rather than qt4-devel.

Actually it's in qt4-x11 but anyway it looks like a packaging bug to me unless something in qt4-x11 actually needs it.  Granted, it's pretty much a harmless one.

> d) It'll make the guidelines simpler as the BuildRequires will just be xemacs,

...or xemacs(bin), depending on the nature of the package being compiled (doesn't make much difference in Fedora buildsys but would be annoying in some local non-mock-and-friends package build scenarios).

> rather than pulling in all of xemacs-devel just for one file.

Well, to me package building (especially rpms) is very much a "development" operation, and I'm pretty sure we already have similar cases with *.pc files in the distro so people should be familiar with it.  BTW currently xemacs-devel is a ~510kB package, so the "all of xemacs-devel" isn't actually too much.  Things could be different if -el is merged to it.

I won't insist on putting the macros file to -devel.  Anyway that's what I'd do if it was entirely my call and no similarities with emacs would be in the equation.

This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

xemacs-21.5.29-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xemacs-21.5.29-4.fc11

I've added this to the main xemacs package as requested.  However, I followed Ville's suggestions for the contents of the file.  Let me know if this needs any further tweaking.

xemacs-21.5.29-4.fc11 has been pushed to the Fedora 11 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 xemacs'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-9964

OK, thanks. Will have a look at it when I get a bit of time and work on making the emacs package consistent and then update the packaging guidelines accordingly. Hve been meaning to get back to this for a while now

Thanks for making this effort.  I think it will make (X)Emacs add-on package specs cleaner and less error prone.

xemacs-21.5.29-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.