Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
This whole thing is a seriously powerful and cool thing. Million thanks for all the work, @mlschroe ! -- 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/846#issuecomment-534031779___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Awesome, 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/846#issuecomment-534030947___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Merged #846 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/846#event-2653832413___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Changed and force-pushed. I also tweaked the code so that a negative number is allowed to make `%[ 3 + %[ 4 - 5]]` work. -- 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/846#issuecomment-534028922___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Yup, that'd make it much easier to understand what it's complaining about. -- 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/846#issuecomment-533990668___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Ah, I get what you mean. I think we should print a bare word error if the
expanded string does not start with a digit. This makes it similar to what
`%{expr:...}` and `%if` does:
```
$ ./rpm --define "aaa a" --define "bbb 123b" --eval '%{expr: %aaa }'
error: bare words are no longer supported, please use "...": a
error:^
$ ./rpm --define "aaa a" --define "bbb 123b" --eval '%[ %aaa ]'
error: macro expansion returned a bare word, please use "...": %aaa
error: ^
error: expanded string: a
$ ./rpm --define "aaa a" --define "bbb 123b" --eval '%[ %bbb ]'
error: macro expansion did not return an integer: %bbb
error: ^
error: expanded string: 123b
```
--
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/846#issuecomment-533576274___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Not sure that message makes it any clearer, I probably failed to explain why I find it confusing to begin with. I guess the problem is that it doesn't explain *why* it expects an integer there, and that makes it sound like it will *only* accept a number there, which in a macro context seems baffling to a macro veteran: ``` $ ./rpm --define "aaa a" --define "bbb b" --eval '%[ 1 < 0 ? %aaa : %bbb ]' error: macro expansion did not return a number: 1 < 0 ? %aaa : %bbb error: ^ error: expanded string: b ``` It seems pretty mysterious (*what do you mean a number, what is this lucky seven it's asking for, macros deal with text!*) until you realize that it should be quoted to be considered a string, or a plain integer. As quotes haven't been prominent in the macro space, I doubt I'll be the only one puzzling over that. That said, I don't have any concrete suggestion for a message offhand, lets sleep over the weekend to see if we can come up with something better. At any rate it I think should talk about an integer instead of a number as only integers are accepted. -- 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/846#issuecomment-533560504___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Sorry for the multiple force pushes, I had a little fight with git. I now use "macro expansion did not return a number" as error message. -- 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/846#issuecomment-533516815___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
@mlschroe pushed 1 commit. 52bd4a99ca452a974b248cf9291454e992e263b3 Implement short-circuit for logical and ternary operators -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/846/files/b0023896a47039a7de8b956b40d0181a9a582991..52bd4a99ca452a974b248cf9291454e992e263b3 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Yeah, looks that little bit nicer that way, thanks. I don't particularly love camelCase but when everything in the surroundings uses it... There's an unrelated indentation change for the division-by-zero case, and trailing whitespace after the doExpressionExpansion() function. I could live with both, just noting for the record. I do regret not re-indenting the whole expression.c to generic rpm style while moving to rpmio/ when the whole thing had been essentially untouched for a decade, doing so now would just mess the recent history unnecessarily. That "contains non-digits" error message baffled me initially, had to fool around with it before getting it. I wonder if something to the tune of "expected an integer" would be more to the point, or maybe something like the bare word error. The expansion output is indeed necessary to make sense out of it regardless of the actual message. Other than those minor nits / pondering, I've no issues with this. Oh and sorry about the conflict (from the double-free thing), care to do one more rebase to resolve that? I promise not to touch expression.c and macro.c again before this is merged :) -- 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/846#issuecomment-533505345___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
I switched the helper function to camelCase and moved the digit check into a separate function to make the code easier to read. -- 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/846#issuecomment-533490784___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
@mlschroe pushed 2 commits. 713d13cb402a76f58be3513fdd8e197754390265 Add support for primary expansion to the expression parser 11e11cf1fc187efc50e4cdff1036355032f97be3 Implement short-circuit for logical and ternary operators -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/846/files/fcc8432f6b5b1a4dbfd8676432252bdc18d27bea..11e11cf1fc187efc50e4cdff1036355032f97be3 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
I also noticed it, but didn't want to check if it's still true ;) I removed that sentence. -- 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/846#issuecomment-533113309___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Thanks. I'll need to take a proper look at this all, but this caught my eye from the docs as it's getting moved around in the patch: > There is currently an 8K limit on the size that this macro can expand to. That's a *little* outdated info, might want to drop it while at it :D -- 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/846#issuecomment-533106765___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
I now also documented the difference to `%{expr:}`
--
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/846#issuecomment-533100283___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
@mlschroe pushed 2 commits. c2469258af1915aeef5b44a6c1444a7a938d99d0 Add support for primary expansion to the expression parser d77df7f2275027228f15b674bbab59d9496adb61 Implement short-circuit for logical and ternary operators -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/846/files/3288db0c688e89131833258d9cac028fabf1e89e..d77df7f2275027228f15b674bbab59d9496adb61 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
I fixed the comment plus added some basic documentation. -- 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/846#issuecomment-533091735___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
@mlschroe pushed 2 commits. 02819e9b723f29329acaed20051f5a0cee10121a Add support for primary expansion to the expression parser 3288db0c688e89131833258d9cac028fabf1e89e Implement short-circuit for logical and ternary operators -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/846/files/616dc88f8e86cbad8d95722ae4357df9008e4df4..3288db0c688e89131833258d9cac028fabf1e89e ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
pavlinamv commented on this pull request. > @@ -1349,6 +1386,13 @@ expandMacro(MacroBuf mb, const char *src, size_t slen) doShellEscape(mb, s, (se - 1 - s)); s = se; continue; + case '[': /* %[...] expression expansion */ + if (mb->macro_trace) + printMacro(mb, s, se); + s++;/* skip ( */ ``` s++;/* skip ( */ ``` -> ``` s++;/* skip [ */ ``` -- 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/846#pullrequestreview-290481295___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Yes, I just ran out of time yesterday but wanted to show your guys the current state. I've added some test cases, next comes the documentation. -- 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/846#issuecomment-533061305___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Yup, some testcases (along with docs) that show the behavior differences to
%{expr:...} would be appreciated.
--
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/846#issuecomment-533035635___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
Cool. It would be great to add a testcase. -- 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/846#issuecomment-533026052___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
@mlschroe pushed 1 commit. 630966ce35c39ac5a8c2a2583099a4165ac15c6f Implement short-circuit for logical and ternary operators -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/846/files/b956786e490c221b64974bdb934af686b49bdc88..630966ce35c39ac5a8c2a2583099a4165ac15c6f ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Support new %[ ] expression expansion syntax (#846)
This adds %[ expr ] as a new means to do expression expansion in rpm. Unlike
%{expr:} the expression is expanded in the parser, so we are safe against
expansion results messing up the syntax and also can to short-circuiting.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/846
-- Commit Summary --
* Add findMacroEnd() function to find the end of a macro call
* Add support for primary expansion to the expression parser
-- File Changes --
M build/parseSpec.c (2)
M rpmio/Makefile.am (2)
M rpmio/expression.c (73)
M rpmio/macro.c (130)
M rpmio/rpmmacro.h (9)
-- Patch Links --
https://github.com/rpm-software-management/rpm/pull/846.patch
https://github.com/rpm-software-management/rpm/pull/846.diff
--
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/846
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
