Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Merged #1414 into master. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#event-3940485078___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Thanks! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-719464028___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
@pmatilai approved this pull request. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#pullrequestreview-520529113___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Oh, sorry I missed that last push from yesterday. Looks fine now, except that the last commit message still talks about mbStartExpansion() and mbFinishExpansion(). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-718919096___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Names are annoying... for a theme that's used elsewhere in rpm too, mbInit() and mbFini() maybe? Also seems to me in that the mbAllocBuf() calls could be moved to the start/init-part too. But yeah much nicer this way, regardless of names. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717768213___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Ok, I factored out the common code. I'm not super happy about the naming, though. Comment welcome ;) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717260763___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
> > spectool was rewritten to use RPM Python bindings, so I imagine at least it > > would matter for that tool. > > Hardly. There's precisely _one_ rpm.expandMacro() in the entire new spectool > code, and that doesn't involve parametric macros or anything else advanced, > it's merely to expand %_sourcedir. That's true, however, it is reasonable to assume that other programs written in Python would need to do this, especially as parametric macros become more common. RPMLint does this in some places too. _I've_ written code to use `rpm.expandMacro()` for personal tools too... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717191415___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
I'll update the patch set after lunch. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717177932___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
So you're suggesting that we remove the argvFree() call in the freeArgs() function? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717171232___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Yet another BTW: I don't think it would be unreasonable to expect rpmExpandThisMacro() callers to supply the macro name as the first argument of the argv (similar to the exec() family of functions) so we wouldn't need to copy that internally just because - the caller knows the name and has to build the argv anyway... With that you could technically even drop the "n" argument, but whether that'd make the interface too weird I dunno. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717145921___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
@mlschroe pushed 1 commit. 6c96d96a091dd97ab52af44ebb09845e94549bf2 Remove common code from expandMacro() and expandThisMacro() -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414/files/91d60615dc967fb4ebd90b05101a389795b2b306..6c96d96a091dd97ab52af44ebb09845e94549bf2 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
@Conan-Kudo , the point is that Python and macros reside on different levels of existence entirely. In the embedded Lua realm, you *are* a macro most of the time, whereas Python only sometimes casually observes the value from the outside. Nobody is forbidding folks from adding more sophisticated macro bindings for Python but in reality the use-cases don't seem to be there. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717214084___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
> spectool was rewritten to use RPM Python bindings, so I imagine at least it > would matter for that tool. Hardly. There's precisely *one* rpm.expandMacro() in the entire new spectool code, and that doesn't involve parametric macros or anything else advanced, it's merely to expand %_sourcedir. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717009198___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
@pmatilai commented on this pull request.
> -
- /* Recursively expand body of macro */
- if (me->body && *me->body) {
- if ((me->flags & ME_LITERAL) != 0)
- mbAppendStr(mb, me->body);
- else
- expandMacro(mb, me->body, 0);
- }
-
- /* Free args for "%name " macros with opts */
- if (args != NULL)
- freeArgs(mb);
- mb->args = prevarg;
- mb->me = prevme;
-
+ doExpandThisMacro(mb, me, args);
Oh, the other thing is that this *looks* a lot like a memory leak, as the argv
free is buried far away from here. I'd prefer symmetry here - args was
initially allocated here so it should also be freed here, ditto with optargs in
expandThisMacro().
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1414#pullrequestreview-517535379___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Okay, just for the reference, I suggested considering using an error code instead of NULL: https://github.com/rpm-software-management/rpm/pull/34 but as to how that turned into -1/1 I plead innocent :D The documented "return negative on error" would allow changing it to return 0 on success like the inner calls do (so you could just pass the inner rc on), I certainly would not mind... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717112044___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Heh, I almost commented on the return code thing, before noticing that it was actually to be same as rpmExpandMacro(). There was a kind of a reason for the return code to be like that, basically to leave room for returning more information in the future (but I don't recall if there was an actual plan as that what exactly). Just had a closer look at the actual patches, my only real complaint is the amount duplication of code between expandMacro() and expandThisMacro(). That's the main difference between your version and mine, and avoids the recursion issues I ran into (so difference between working and non-working :grin: ), but... maybe some of the entry/exit mb checks could be further refactored to use common code? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717105529___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
I wonder if we should make the interface more sane. I currently made it similar to rpmExpandMacro(), but that was probably a bad idea. The interface is currently return 1 on success and -1 on failure, put string in the obuf pointer. But the straightforward interface would be return string on success, NULL on failure. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-717090028___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
@pmatilai `spectool` was rewritten to use RPM Python bindings, so I imagine at least it would matter for that tool. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-716920555___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Oh and BTW, there's actually an existing use-case for this in the code-base: this is exactly what runCall() in build/rpmfc.c wants. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-716536974___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
...except that's exactly where it starts getting trickier: from Lua the argv is differently allocated and shouldn't be freed in freeArgs() and at least in my version there was "fun" with me->opts processing retriggering the option processing when it shouldn't have, etc. But many details are different here, I'll rebase the Lua work on top of this and see how it looks like now. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-716507277___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
I can pick up the Lua side once this is merged, no need to redo all that. As for Python, the bindings wrt macros are really primitive as it is and nobody is complaining so I dunno if anybody cares. At any rate, I don't consider that a requirement for merging. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-716498314___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
This needs to be connected to python/lua as well. How should the interface look like? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-716487117___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add new rpmExpandThisMacro() public method (#1414)
Ah, making a copy of the argv so the allocation business is different from what I had. https://github.com/pmatilai/rpm/tree/luamt-2 is a quick "port" on top of this and the basic testcases pass as-is, but only very briefly tested. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1414#issuecomment-716521957___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
