Bug 1279723 - spectool rewrite
Summary: spectool rewrite
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: rpmspectool
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Nils Philippsen
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-11-10 06:29 UTC by Jason Tibbitts
Modified: 2016-02-19 14:57 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-02-19 00:39:11 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1284000 0 medium CLOSED Review Request: rpmspectool - Utility for RPM spec files 2021-02-22 00:41:40 UTC

Description Jason Tibbitts 2015-11-10 06:29:50 UTC
I've always had some nagging issues wih spectool, specifically;

* -R doesn't work if your %_sourcedir is defined using macros which need to be expanded,

* spectool just completely falls apart if you use macros in SourceN: or PatchN: outside of a limited set, or if it simply can't figure out the values for those macros.

The second is more important as we're looking into refining our Python packaging to introduce macros and templates which would, in the default case, break spectool.  Since spectool is somewhat important, I set out to fix it but the source wasn't really pleasant so I ended up rewriting it,

I rewrote it in Python3, not out of any Perl hate (I wrote a lot of Perl) but because that seems to be the language of choice for Fedora these days and I've been doing most of my work in it lately.

The script (just one file, though I could split it up) is at https://pagure.io/python-macros/blob/master/f/spectool-py

It should have identical output, behavior and return values to the original spectool, save for debugging and help output.  At this point I see any deviation arising from regular use as a bug, though eventually I would like for the tool to evolve beyond that restriction.

Please let me know if there's a possibility of including this rewrite in upstream rpmdevtools in some fashion, and what I could do to facilitate that.

Comment 1 Ville Skyttä 2015-11-11 08:52:18 UTC
(In reply to Jason Tibbitts from comment #0)
> Please let me know if there's a possibility of including this rewrite in
> upstream rpmdevtools in some fashion, and what I could do to facilitate that.

It's certainly possible. But what I want is that you or someone else would "sign up" for maintaining it, I have too little time already for the time being. And if there's an active maintainer for it, why not make it a separate project? IIRC there's nothing spectool requires from rest of rpmdevtools, ditto the other way around. WDYT?

Regarding the actual code, Python 3 is very much fine, but have you looked into using the rpm and curl Python bindings instead of shelling out to executables?

Comment 2 Jason Tibbitts 2015-11-12 20:34:04 UTC
I'm perfectly happy to maintain this code, though of course I'll pull it out into its own repository.

I'm not sure if the rpm bindings would give me what I need, but the curl bindings would certainly be preferable (and there's a comment in the code about that).  I was, however, trying to duplicate the spectool behavior exactly, and that includes shelling out so that the output is identical.  I'm not yet sure if I can use the internal curl bindings and still preserve the usage of /etc/rpmdevtools/curlrc without actually parsing that file manually.

Comment 3 Jason Tibbitts 2015-11-12 20:50:31 UTC
A bit of searching shows me that I could potentially replace rpm --eval with rpm.expandMacro(), but that the SpecParse doesn't appear to be exposed.  There isn't much documentation here so I actually have no idea at all  how I would actually call expandMacro with a spec file provided, but if you have any pointers I'd be happy to read more about this.

Comment 4 Ville Skyttä 2015-11-15 17:18:25 UTC
Regarding spec parsing in python:

import rpm
spec = rpm.spec("/path/to/foo.spec")

Then, if that succeeds, spec.sources is a list of tuples like:
    (source_url, X, Y)
...where source_url is the URL to the SourceX or PatchX (yes, spec.sources contains them both), X is the SourceX/PatchX number, and Y I believe is 2 for patches, 1 for sources (and I suppose there are some nice rpm.SOMETHING constants for them). There's also spec.sourceHeader which I believe is a "fake" source header for the specfile, as if it was a srpm, additional stuff can be grabbed from it if required.

I don't know if it is possible to define some values for spectool -D, maybe rpm.addMacro() before calling rpm.spec()?

Anyway, how would you like us to proceed with this? I'm fine with either splitting spectool out of rpmdevtools, or keeping it there and giving you commit access for maintaining it there.

Comment 5 Jason Tibbitts 2015-11-16 21:31:03 UTC
Yeah, I have found the source for yum-builddep which has a bit of code for dealing with this; it will take more experimentation but I think I should be able to avoid at least some shelling out.  If I can get rid of it entirely then it would shrink the code a bit.

I currently have spectool in its own little repo at https://pagure.io/spectool.  Seems reasonable to leave it there, but it is entirely up to you.  I am happy to keep it a part of your source tree or even to push it through package review and maintain it as a completely separate package.  I suppose the latter would mean that you wouldn't see any bug reports, which may be desirable for you.

Comment 6 Nils Philippsen 2015-11-20 14:35:05 UTC
Hi everybody. Ville was so kind to point me to this BZ ticket :).

Apropos of nothing special I've just (in the recent days) rewritten spectool in Python (3) which is something I wanted to do for a long time (since I haven't written anything serious in Perl since, well, spectool in 2004). 

Anyway, I've given the rewrite a different name -- rpmspectool -- so it doesn't need to clash with the original version, and so I don't have to stay 100% command line compatible ;). This incidentally fixes --sourcedir/-R at least for default setups -- I haven't put much effort into breaking it by playing shenanigans with macros. It also uses pycurl/libcurl directly instead of calling curl from the command line.

Github: https://github.com/nphilipp/rpmspectool
PyPI: https://pypi.python.org/pypi/rpmspectool
Fedora review: https://bugzilla.redhat.com/show_bug.cgi?id=1284000

My idea was to package it separately and (if this is wanted) let rpmdevtools contain a thin shim which just translates command line options to the new calling convention. What do you think?

Comment 7 Nils Philippsen 2015-11-23 10:07:44 UTC
Updates-testing and Rawhide should have packages of rpmspectool shortly. Can you give them a whirl?

F-23: https://bodhi.fedoraproject.org/updates/FEDORA-2015-b73e615d53
F-22: https://bodhi.fedoraproject.org/updates/FEDORA-2015-55ff2322ad

Jason, do you have test cases for this second issue of yours?

(In reply to Jason Tibbitts from comment #0)
> * spectool just completely falls apart if you use macros in SourceN: or
> PatchN: outside of a limited set, or if it simply can't figure out the
> values for those macros.

Comment 8 Ville Skyttä 2015-11-23 12:17:47 UTC
(In reply to Nils Philippsen from comment #6)
> My idea was to package it separately and (if this is wanted) let rpmdevtools
> contain a thin shim which just translates command line options to the new
> calling convention. What do you think?

I don't have too warm feelings about this. There can be a compatibility shim, but let's have it wherever the actual *tool implementation lives. I can place some Obsoletes in rpmdevtools that should bring in the new one on upgrades (assuming dnf works the same in this regard as yum did).

And just MHO, I think I'd prefer to have the tool named spectool also in the future, wherever it is shipped, unless there is a known conflict with another tool. The rpm* namespace is already too crowded.

Comment 9 Nils Philippsen 2015-11-23 18:37:49 UTC
(In reply to Ville Skyttä from comment #8)
> I don't have too warm feelings about this. There can be a compatibility
> shim, but let's have it wherever the actual *tool implementation lives. I
> can place some Obsoletes in rpmdevtools that should bring in the new one on
> upgrades (assuming dnf works the same in this regard as yum did).

Fine with me, and obsoletes have worked the same for me regardless of whether yum or dnf was used.

> And just MHO, I think I'd prefer to have the tool named spectool also in the
> future, wherever it is shipped, unless there is a known conflict with
> another tool. The rpm* namespace is already too crowded.

Hm, then (rpm)spectool should just provide the 'old' calling convention, maybe (IMO ideally) marked as "deprecated" or something like that. I need to look if and how argparse can be cajoled into doing that "nicely", i.e. --help would show the new calling convention only and so on.

Comment 10 Jason Tibbitts 2015-11-23 19:41:06 UTC
Hmm, well, I tried to inform the world that I was working on spectool, but oh, well.  One less thing for me to do and your version is probably far better than mine anyway.  Too bad about the duplication of work, though.  

I have run into various random specfiles where spectool couldn't parse the Source: stuff.  I can't recall all of them, but I know it did poorly when the macros were defined in Lua.  That happens to be the case with the new python macro stuff FPC has been working on.  It's not trivial to test those out at the moment but when the package hits F23 I can do some testing.

Comment 11 Nils Philippsen 2015-11-25 10:38:35 UTC
Yeah, sorry for the unnecessary work :(. Please poke me when you have something exhibiting the problems you mentioned.

Comment 12 Nils Philippsen 2015-11-25 11:11:35 UTC
NB: this should probably be reassigned to rpmspectool then. Do you concur?

Comment 13 Ville Skyttä 2015-11-25 11:43:39 UTC
Sure, fine with me

Comment 14 Nils Philippsen 2015-12-01 10:18:38 UTC
Jason, rpmspectool-1.99.3 is in F23 now, currently in testing, but I've submitted it to stable despite some little warts (handling of RPM, download errors). Please install it and check how it fares with your problematic spec files:

rpmspectool list <name-of-your-package>.spec

Comment 15 Jason Tibbitts 2016-02-19 00:39:11 UTC
Seems to work OK on the weird stuff I tested, though it's unfortunate that the rewrite couldn't maintain at least some of the command line options.  Without that it seems that we're stuck with this and the old spectool basically forever.

I think if rpmspectool -g *spec worked then we could probably handle renaming it to spectool, removing spectool from rpmdevtools, adding a dependency and getting on with life.

Comment 16 Ville Skyttä 2016-02-19 14:57:50 UTC
As said earlier, renaming+dropping would be my preference as well. In any case, I do not intend to keep shipping the current spectool in rpmdevtools forever. I've tried to get used to using rpmspectool myself and it has worked very well for me, but I really prefer the (non-rpm* prefixed) name of spectool as well as its command line usage over rpmspectool's, not to mention lack of bash completion for the latter...


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