Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
A lot of related code has changed in the meanwhile and this PR carries an unnecessary amount of historical baggage by now. I'll close this one, @pavlinamv do create a new PR for restarting the %elif work to get a fresh start. -- 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/613#issuecomment-493973282___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Closed #613. -- 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/613#event-2353002024___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
This is on hold for now, @pavlinamv has more important assignments at the moment. -- 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/613#issuecomment-469203346___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Thank you for pointing out that this should be rebased. I have a slightly different plan. #625 already contains a non-trivial refactorization, that is useful for %elif. My next step will be to create an additional PR that will contain a correction of a current minor %else parsing problem + as a side effect a part of code that will be usefull for %elif too. After that I will rebase this PR. -- 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/613#issuecomment-469185640___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
@pavlinamv #625 was merged, so could you rebase this on top of current git 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/613#issuecomment-468939640___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
I am waiting with a new version of this PR for finishing of PR #625. -- 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/613#issuecomment-465034301___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Oh and BTW, a general piece of advise that applies everywhere, including here: when possible, use data structures instead of code logic. -- 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/613#issuecomment-460593166___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
As mentioned in the warn-after-else PR, one of the things that bugs me here is multiple places to parse the same tokens. Have a function that parses the "line type", which you can then use in both place, maybe borrow a chapter from the macro engine builtins work? The other coding style eyesore is elifEnabled, which just sticks out like a sore thumb (there's no ifEnabled, elseEnabled or endifEnabled). Find a way to do the branch-state tracking in nice, concise and consistent manner for everything. And I don't mean adding separate fooEnabled variables for them all. Obviously %elif is an useful construct as such but my intuition has been against it all along, it's just that I haven't been able to pinpoint a concrete reason. The findings in #625 perhaps hint at why: one shouldn't build on a swamp without draining it first. As in, eliminate quirks and actually define the expected behavior before attempting to add anything new. -- 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/613#issuecomment-460571560___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Reopened #613. -- 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/613#event-2117852085___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
The pull request is reopened (PR #618 will be closed). -- 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/613#issuecomment-460559995___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request.
> spec->readStack = rl;
spec->line[0] = '\0';
+} else if (isElif) {
+ spec->readStack->reading = match && spec->readStack->readable;
+ if (spec->readStack->reading)
+ spec->readStack->readable = 0;
+ spec->line[0] = '\0';
+ match = -1;
Well, I wouldn't be asking for you to rethink over and over again if *I* was
happy with the code.
Which is why I've been asking for you to rethink at a more fundamental level,
fundamental meaning something deeper than moving around a few lines of existing
code.
So we have a bit of a stalemate here: if you can't see anything wrong with this
code then clearly I'm not going to get anything better by asking for a rethink.
--
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/613#discussion_r248266809___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Corrected version is in PR #618. -- 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/613#issuecomment-453772152___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Closed #613. -- 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/613#event-2070220111___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pavlinamv commented on this pull request.
> spec->readStack = rl;
spec->line[0] = '\0';
+} else if (isElif) {
+ spec->readStack->reading = match && spec->readStack->readable;
+ if (spec->readStack->reading)
+ spec->readStack->readable = 0;
+ spec->line[0] = '\0';
+ match = -1;
> I wasn't asking for detailed explanation against my remarks, just pointing
> (partial) overlap and throwing possible ideas to explore.
I think that if you had a concrete remark and I am not planning to involve it
in next version of PR, then it is fair to explain why. And usually it is better
to explain it precisely.
> Like said: knowing what's involved and the surrounding code, rethink from
> scratch and see what comes up, you can almost always do better the second or
> even third time around.
I rethink this code many times and for me it results in the refactorization
patches before the "elif" patch.
> You mentioned corner cases that this doesn't take into account - take them
> into account from the beginning the in the next round.
Yes I mentioned those corner cases. Of course I planned to solve them - but in
following PR. There is no gain from solving it in this PR.
> I'm absolutely positive that if-elif-else-endif branching can be expressed
> and tracked in a more coherent manner than we have here. As discussed in
> private this ugly code to begin with, take the opportunity to improve the
> foundation instead of duct-taping over it. Sometimes there's no choice but to
> duct-tape due to time or API constraints but neither is an issue here.
If you have some concrete issues or improvements of the PR, please send me
them. But I am happy with this 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/613#discussion_r247311274___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request.
> spec->readStack = rl;
spec->line[0] = '\0';
+} else if (isElif) {
+ spec->readStack->reading = match && spec->readStack->readable;
+ if (spec->readStack->reading)
+ spec->readStack->readable = 0;
+ spec->line[0] = '\0';
+ match = -1;
I wasn't asking for detailed explanation against my remarks, just pointing
(partial) overlap and throwing possible ideas to explore. Like said: knowing
what's involved and the surrounding, rethink from scratch and see what comes
up, you can almost always do better the second or even third time around. You
mentioned corner cases that this doesn't take into account - take them into
account from the beginning the in the next round. I'm absolutely positive that
if-elif-else-endif branching can be expressed and tracked in a more coherent
manner than we have here. As discussed in private this ugly code to begin with,
take the opportunity to improve the foundation instead of duct-taping over it.
Sometimes there's no choice but to duct-tape due to time or API constraints but
neither is an issue here.
--
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/613#discussion_r246735540___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pavlinamv commented on this pull request.
> spec->readStack = rl;
spec->line[0] = '\0';
+} else if (isElif) {
+ spec->readStack->reading = match && spec->readStack->readable;
+ if (spec->readStack->reading)
+ spec->readStack->readable = 0;
+ spec->line[0] = '\0';
+ match = -1;
Thank you for the comment.
> For example, isn't elifEnabled practically the same thing as checking
> whether ->next non-NULL?
No. Because `elifEnabled = 1` if some `%if` condition starts and does not reach
`%else` or `%endif`. `->next` is non-NULL if some `%if` starts and does not
reach `%endif`. And from all other `->` variables you can not decide whether in
the current `%if` `%else` was reached or not. Yes the meaning and name of the
variable can be different, but there must be some variable containing this
information.
> Also makes me wonder if it'd fit in more naturally if you just treated
> %elif as a new %if inside an %else, which is what it ultimately is.
> %elif is just syntactic sugar after all.
This alternative approach does not optimize variables, or complexity of the
program:
Programming `%elif` like a "special `%else` `%if` " will lead into:
- `->elifEnabled` must stay in the code with this approach too. (Of course no
metter how it is implemented, there is need for testing whether all %elif's are
on their propper places).
- even if in this approach `->reading` can be omitted, there must be some new
variable counting how nested the `%elif` is. It is because after `%else` resp.
`%endif`, function `readLine`, must close the main `%if`, and all other
"auxiliary %else %if's" that emulate `%elifs`.
Thus it will result into code that is not better than the current one.
Current approach corresponds to defining %elif straightforwardly according to
its syntax and the alternative approach corresponds to defining %elif using %if
inside an %else. Both are correct definitions, but for me the current looks
more readable and natural.
--
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/613#discussion_r246720538___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pavlinamv commented on this pull request.
> @@ -360,7 +364,10 @@ do { \
int readLine(rpmSpec spec, int strip)
{
char *s;
-int match;
+char *z;
+int match = 0;
+int isIf = 0;
+int isElif = 0;
Yes a line can be `%if` line, `%elif` line or "another" line.
Yes, in some earlier version were only two variables: `isElif` + `match`. The
meaning of variable `match` was quite unclear.
` 0 %if or %else line + condition after %if is not satisfied`
` 1 %if or %else line + condition after %if is satisfied`
`-1 ... %if or %else line + condition after %if is bad, or `
` .. not %if or %elif line`
`match` was used for both: deciding whether line is `%if` line and deciding,
whether the condition after `%if` (resp. `%ifos`, ...) is satisfied.
That is why I made the first patch of this PR. In the patch I refactored
`match` into two variables: `match` + `isIf` to have clean meaning and usage.
Variables `isIf` and `isElif` can be consolidated into one. I will change it.
--
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/613#discussion_r246402912___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request.
> spec->readStack = rl;
spec->line[0] = '\0';
+} else if (isElif) {
+ spec->readStack->reading = match && spec->readStack->readable;
+ if (spec->readStack->reading)
+ spec->readStack->readable = 0;
+ spec->line[0] = '\0';
+ match = -1;
Sorry but this all still seems like something duct-taped to the side to me.
Like I said earlier in private: now that you know what's involved, please
rethink the approach from scratch - what you really need tracked and where, and
how to best achieve these things in a clear and concise manner. One can almost
always do a better job the second time around.
There would seem to be a fair amount of overlap in these various tracking
variables, some local, some wider, that could be eliminated / combined. For
example, isn't elifEnabled practically the same thing as checking whether
->next non-NULL? Also makes me wonder if it'd fit in more naturally if you just
treated %elif as a new %if inside an %else, which is what it ultimately is.
%elif is just syntactic sugar afterall.
--
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/613#pullrequestreview-190734941___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request.
> @@ -360,7 +364,10 @@ do { \
int readLine(rpmSpec spec, int strip)
{
char *s;
-int match;
+char *z;
+int match = 0;
+int isIf = 0;
+int isElif = 0;
A line cannot be both an if and an elif, so there should only really be one
variable to track it. I think some earlier version had something like that,
dunno why you dropped it.
--
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/613#pullrequestreview-190718012___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
If this comment is about patch "Move checking whether %if condition will be resolved to the right place" then the reply is: I will improve the commit message. It should not speak about the "right place". But it is not just refactoring because of `%elif`. The current place is not optimal because: In function `readLine()`, all of conditionals (`%else`, `%endif`, `%ifarch`, `%ifnarch`, ...) except for `%if` are parsed in one place. But `%if` is parsed in two different places (there is `ISMACROWITHARG(s, "%if")` twice). I think that it is better to consolidate these two `ISMACROWITHARG(s, "%if")` into one place. And after that optimize it to be coded similarly as `%ifarch`, `%ifos`, ... -- 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/613#issuecomment-452327087___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pavlinamv commented on this pull request.
> lbuf = spec->lbuf;
SKIPSPACE(lbuf);
if (lbuf[0] == '#')
isComment = 1;
- /* Don't expand macros (eg. %define) in false branch of %if clause */
-if (!spec->readStack->reading)
+/* Don't expand macros after %elif (resp. %elifarch, %elifos) in false
branch */
+if (ISMACROWITHARG(lbuf, "%elif")) {
+ if (!spec->readStack->readable)
The need to test for `ISMACROWITHARG(lbuf, "%elif")` in that place is caused by
the current implementation of the parser and not by tracking by "reading" and
"readable".
Parsing of a line in spec file consists of these for us important actions (in
this order):
1) storing the line from file into buffer
(function `copyNextLineFromOFI`)
2) deciding whether the line should be expanded
(beginning of function `expandMacrosInSpecBuf`)
3) if it should be expanded then expansion of the line
(second part of function `expandMacrosInSpecBuf`)
4) checking if the line starts with a rpm conditional and according of it
doing appropriate actions
(second part of function `readLine`)
Before adding `%elif`, the decision 2) whether the line should be expanded
depends only on previous lines of the spec file. But after adding `%elif` it
depends on the new line stored in the buffer, too. Because if the line starts
with `%elif` then `if !readable` line must not be expanded, otherwise it must
be expanded.
If there should be a variable that is used for deciding whether the new line
should be expanded or not, then the variable must be set after 1). So it can be
set
a) in the second part of function `copyNextLineFromOFI` before calling
`expandMacrosInSpecBuf`, or
b) in the first part of `expandMacrosInSpecBuf`.
In the current patch the test is done in position b) and instead of setting a
variable and then deciding according to it, the test is straightforwardly used.
Thus I do not see any improvement in changing tracking variables to allow
decision 2).
To be precise I should add into `expandMacrosInSpecBuf` tests for
`ISMACROWITHARG(s, "%elifarch")` and `ISMACROWITHARG(s, "%elifos")` too. And to
be extremely precise with solve all corner cases I should add `ISMACRO(s,
"%endif")` and `ISMACRO(s, "%else")` too. Because they all can change the
potential to expand in that line. ( But usually only after `%elif` is expected
to be a macro).
--
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/613#discussion_r246021478___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pavlinamv commented on this pull request.
> @@ -421,8 +422,8 @@ int readLine(rpmSpec spec, int strip)
match = parseExpressionBoolean(s);
if (match < 0) {
rpmlog(RPMLOG_ERR,
- _("%s:%d: bad %%if condition\n"),
- ofi->fileName, ofi->lineNum);
+ _("%s:%d: bad %%if condition\n expanded line:
%s"),
+ ofi->fileName, ofi->lineNum, z);
Yes, there is a mistake in the commit message, it should talk about %if, not
about %elif. In the newer version of the PR I will correct this patch to have
common style and its commit 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/613#discussion_r246014711___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request. How can the current place be "wrong" and the new place "right" if there's no functional change? If it's just refactoring for the next steps, then the commit message should say so. -- 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/613#pullrequestreview-189744282___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request.
> @@ -421,8 +422,8 @@ int readLine(rpmSpec spec, int strip)
match = parseExpressionBoolean(s);
if (match < 0) {
rpmlog(RPMLOG_ERR,
- _("%s:%d: bad %%if condition\n"),
- ofi->fileName, ofi->lineNum);
+ _("%s:%d: bad %%if condition\n expanded line:
%s"),
+ ofi->fileName, ofi->lineNum, z);
The common style is to just output the line after a colon and without ado about
expansions, ie
```
_("%s:%d: bad %%if condition: %s")
```
Lets stick to that please. Couple of other nits:
- why include the possible preceding whitespace in the error message?
- the commit message talks about %elif message but support for that hasn't been
added yet
--
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/613#pullrequestreview-189743396___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
pmatilai commented on this pull request.
> lbuf = spec->lbuf;
SKIPSPACE(lbuf);
if (lbuf[0] == '#')
isComment = 1;
- /* Don't expand macros (eg. %define) in false branch of %if clause */
-if (!spec->readStack->reading)
+/* Don't expand macros after %elif (resp. %elifarch, %elifos) in false
branch */
+if (ISMACROWITHARG(lbuf, "%elif")) {
+ if (!spec->readStack->readable)
As mentioned in earlier private comments, the need to test for
ISMACROWITHARG(lbuf, "%elif") here seems out of place, and suggests that the
if-elif-else branching logic needs some better form of tracking than the
current combo of reading + readable.
--
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/613#pullrequestreview-189740756___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Conan-Kudo 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/613#pullrequestreview-189678463___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
There's no mechanism for tracking build-side features, we just let things explode. -- 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/613#issuecomment-448563816___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)
Won't we need an `rpmlib()` dependency emitted for source packages that have `%elif`? Or do we just typically ignore this and let things explode on their own? -- 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/613#issuecomment-448423302___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
