Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Basically the closest example from the C language would be `#define`. You have to escape line breaks the same way, leading to the same readability issues if done extensively :) -- 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/issues/1198#issuecomment-679977879___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
> Eliminating ambiguity (which is _always_ buggy from somebody's perspective)
> is usually worth a fair amount of disruption in the end, and messy is in the
> eye of the beholder.
>
> ```
> %define test() \
> %if 1\
> BUG\
> %endif\
> %{nil}
> ```
>
> It's not that obvious whether the %if is meant for the spec parser inline, or
> whether it's meant to be part of the macro body.
Is that a question for the parser or for the person reading the code?
If the former, that's not really the case, as it's unambiguously part of the
macro body, due to the line-continuation marker(s) preceding it. By
"collapsing" the `%define` the whole body becomes one line and the parser
continues past it.
If the latter, totally agree. But I think this is the case with any other
language as well; whenever you manually line-break statements, consider e.g. a
long `if` expression in C. It's more about the "code culture" at that point.
For specs, it's not usual to indent stuff, but that probably has to do with
insufficient support for that (is that still the case, btw?).
--
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/issues/1198#issuecomment-679974112___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Eliminating ambiguity (which is *always* buggy from somebody's perspective) is
usually worth a fair amount of disruption in the end, and messy is in the eye
of the beholder.
```
%define test() \
%if 1\
BUG\
%endif\
%{nil}
```
It's not that obvious whether the %if is meant for the spec parser inline, or
whether it's meant to be part of the macro body.
```
%define test() \
%%if 1\
BUG\
%%endif\
%{nil}
```
This is certainly not *pretty*, but it does at least hint at %if being special
here. And in fact it does seem to do the right thing as-is (although just
briefly tested) so the only thing needed would be adding a warning about
ambiguous %if and friends when not escaped, somehow.
--
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/issues/1198#issuecomment-679950861___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Yes, this is really ugly :) It turns out, though, these `%define` & `%if` constructs are not that rare after all: https://src.fedoraproject.org/rpms/kernel/blob/master/f/kernel.spec#_2778 I wonder how much disruption it would be for such packages if we start requiring proper escaping. Also, this could make spec files look a bit more messy than they already are. -- 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/issues/1198#issuecomment-679931880___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Thinking some more, it might not be such a bad idea in general to require escapes for ambiguous constructs. There's all manner of weird behavior in this neighbourhood, for a loosely related example: ``` %global test() \ %description %description %test ``` This produces an empty description in the package, no warnings, and removing the %description above the %test line produces identical output :zany_face: -- 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/issues/1198#issuecomment-679923082___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Having looked at the PR, I think the right solution to this would be requiring %if and its relatives escaped for this kind of usage within the spec, and error out otherwise. How easy (or not) that is to achieve, I don't know. -- 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/issues/1198#issuecomment-679890520___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
So... I was about to ask how come this then works:
```
%{?__debug_package:%{__debug_install_post}}\
%{__arch_install_post}\
%{__os_install_post}\
%{nil}
```
...but the answer is that *it doesn't*. The debug foobar gets appended to
noarch package %install too, contrary to obvious intention of the original
author. And okay, this can easily be witnessed by the debug*.list files
appearing in the build directory of a noarch package, they're just unused as
the %debug_package macro expands to nothing on noarch packages.
Wacko :sweat_smile:
--
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/issues/1198#issuecomment-675357458___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Having revisited this again, I think I have a better grasp of the whole
mechanism now. And it's way simpler than I originally thought.
First of all, there's no such thing as "support for conditionals inside macro
definitions". Macros are just that - they may contain arbitrary text to be
substituted by them, including other macros which are expanded recursively.
Now, since `%if` is not a macro per se, it won't get expanded into anything and
just pushed to the parser for further processing (which includes conditional
parsing). As a result, spec directives found inside conditionals (that are
found inside macros) are a valid construct.
In fact, we ourselves have been using this *for years* in the `%debug_package`
macro shipped with RPM:
```
%debug_package \
%ifnarch noarch\
%global __debug_package 1\
%_debuginfo_template\
%{?_debugsource_packages:%_debugsource_template}\
%endif\
%{nil}
```
That being said, there's a catch. Since conditionals are only evaluated *after*
recursive macro expansion, the `%__debug_package` macro above will *always* be
set to 1, regardless of whether `BuildArch` is `noarch`. I think this was
(quite understandably) misunderstood when the macro was written. Another
example is the one I gave in the previous comment where "hello" is always
printed due to the fact that `echo` gets expanded (and executed) before the
conditional is.
In any case, bringing back the original functionality makes sense after all,
and I'll post a PR shortly.
--
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/issues/1198#issuecomment-675338506___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
This issue stems from the fact that the line continuation marker `\` has
*different* semantics in the spec-level context and in a macro definition. On
the spec level, it is used to break long `%if` statements into multiple lines.
Inside macro definitions, it's the whole body that's broken down. The patch in
5f4fdce interprets these markers equally, though.
This becomes a problem when a `%define` or `%global` macro is encountered in a
false branch of an `%if` statement and is therefore left unexpanded (for
obvious reasons); the spec parser just continues scanning the macro's body as
if it were part of the spec itself: it joins the inner `%if` statement into a
single line, including the inner `%endif` (since all of the lines end with an
`\`) and finally tries to match it against a corresponding `%endif`, which
fails as there's none.
The remedy here would be to completely skip the macro's body in case it's
unexpanded. In fact, there's a very simple and elegant (yet non-obvious) way to
do this: when joining multi-line `%if` conditionals, treat `%define` as a such
a conditional; that way, the whole macro (when unexpanded) collapses into a
single line and its content is not interpreted. I have a working patch here:
https://github.com/dmnks/rpm/commit/9c1c592d40777868d672a531b49c63fb6dd6ec84
That being said, it turns out that conditionals inside macros are
*unsupported*. The macro expander does *not* interpret them at all. For
example, the following construct would print "hello":
```
%define test() \
%if 0 \
%{echo:hello} \
%endif
%test
```
This is also mentioned (though not entirely clearly) in the `/doc/manual/spec`
file:
```
%if-conditionals are not macros, and are unlikely to yield expected results
if used in them.
```
@ignatenkobrain, is there a specific use case for such conditionals that I'm
missing? While there is a simple fix for this (as described above) which
shouldn't introduce any side-effects (but I am not 100% sure yet), it doesn't
make much sense to apply it if the use case being fixed is in fact unsupported.
--
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/issues/1198#issuecomment-660079015___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
Ack, thanks for chasing that down. -- 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/issues/1198#issuecomment-619789939___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
5f4fdce3041b80ef26e3a87db07ecdbd2041a9b2 is the first bad commit commit 5f4fdce3041b80ef26e3a87db07ecdbd2041a9b2 Author: Pavlina Moravcova Varekova Date: Mon Jul 1 13:06:35 2019 +0200 Parse multiline conditional and %include parameters correctly (#775) Trailing '\' after multiline conditionals or %include will be trimmed. After the patch lines like: %if 1 && ( 2 || \ 3 ) %endif will be parsed correctly. The other lines stay unchanged. A test is added. build/parseSpec.c | 25 +++-- tests/Makefile.am | 1 + tests/data/SPECS/ifmultiline.spec | 28 tests/rpmmacro.at | 12 4 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 tests/data/SPECS/ifmultiline.spec -- 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/issues/1198#issuecomment-619787090___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
It also happens in 4.15, but not 4.14. -- 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/issues/1198#issuecomment-619785696___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Unclosed %if (when defined inside %define) (#1198)
```
Name: hello
Version: 1
Release: 1%{?dist}
Summary: Hello
License: Public Domain
URL: https://google.com
%if 0%{?foo} >= 8
%define test() \
%if 1\
BUG\
%endif\
%{nil}
%endif
%description
%{summary}.
%prep
%autosetup -c -T
%files
```
Make sure that `foo` is not defined in your environment.
This produces:
```
error: line 9: Unclosed %if
```
This is happening with `rpm-build-4.15.90-0.git14971.8.fc33.x86_64`.
--
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/issues/1198___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
