Unclosed %if (when defined inside %define) #1198
Comments
|
It also happens in 4.15, but not 4.14. |
|
5f4fdce is the first bad commit |
|
Ack, thanks for chasing that down. |
|
This issue stems from the fact that the line continuation marker This becomes a problem when a 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 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": This is also mentioned (though not entirely clearly) in the @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. |
|
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 In fact, we ourselves have been using this for years in the Here, That being said, there's a catch. Since conditionals are only evaluated after recursive macro expansion, the In any case, bringing back the original functionality makes sense after all, and I'll post a PR shortly. |
|
So... I was about to ask how come this then works: ...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 😅 |
Since the body of a newly defined macro may span multiple lines and contain %if expressions, we need to make sure the line parser does not try to interpret those when the corresponding %define or %global macro appears in a false %if branch and is therefore left unexpanded in the line buffer. This is usually not a problem since any macros found in the body itself would not be expanded anyway, but it can break the syntax check for conditionals, which follows after expansion. This is because, with the recently introduced support for line-continuation markers in %if expressions (commit 5f4fdce), the parser would be tricked into thinking that the markers belong to the %if expression itself and collapse them into a single line, including the matching %endif, and then complain about the missing %endif (which must be on a separate line). Instead, similarly to %if, we should collapse the %define/%global macro itself so that the line parser doesn't pass through the body at all. A side effect of this change (of commit 5f4fdce from a year ago, in fact) is that %if constructs are no longer syntax-checked within macros defined in false %if branches, but that's arguably correct behavior, since they only come into existence when their definition macro (%define or %global) is actually expanded, and should merely be treated as text values until that point. Fixes: rpm-software-management#1198
Since the body of a newly defined macro may span multiple lines and contain %if expressions, we need to make sure the line parser does not try to interpret those when the corresponding %define or %global macro appears in a false %if branch and is therefore left unexpanded in the line buffer. This is usually not a problem since any macros found in the body itself would not be expanded anyway, but it can break the syntax check for conditionals, which follows in readLine() after expansion. This is because, with the recently introduced support for line-continuation markers in %if expressions (commit 5f4fdce), the parser would be tricked into thinking that the markers belong to the %if expression itself and collapse them into a single line, including the matching %endif, and then complain about the missing %endif (which must be on a separate line). Instead, similarly to %if, we should collapse the %define/%global macro itself so that the line parser doesn't pass through the body at all. A side effect of this change (of commit 5f4fdce from a year ago, in fact) is that %if constructs are no longer syntax-checked within macros defined in false %if branches, but that's arguably correct behavior, since they only come into existence when their definition macro (%define or %global) is actually expanded, and should merely be treated as text values until that point. Fixes: rpm-software-management#1198
Since the body of a newly defined macro may span multiple lines and contain %if expressions, we need to make sure the line parser does not try to interpret those when the corresponding %define or %global macro appears in a false %if branch and is therefore left unexpanded in the line buffer. This is usually not a problem since any macros found in the body itself would not be expanded anyway, but it can break the syntax check for conditionals, which follows after expansion. This is because, with the recently introduced support for line-continuation markers in %if expressions (commit 5f4fdce), the parser would be tricked into thinking that the markers belong to the %if expression itself and collapse them into a single line, including the matching %endif, and then complain about the missing %endif (which must be on a separate line). Instead, similarly to %if, we should collapse the %define/%global macro itself so that the line parser doesn't pass through the body at all. A side effect of this change (of commit 5f4fdce from a year ago, in fact) is that %if constructs are no longer syntax-checked within macros defined in false %if branches, but that's arguably correct behavior, since they only come into existence when their definition macro (%define or %global) is actually expanded, and should merely be treated as text values until that point. Fixes: rpm-software-management#1198
Since the body of a newly defined macro may span multiple lines and contain %if expressions, we need to make sure the line parser does not try to interpret those when the corresponding %define or %global macro appears in a false %if branch and is therefore left unexpanded in the line buffer. This is usually not a problem since any macros found in the body itself would not be expanded anyway, but it can break the syntax check for conditionals, which follows after expansion. This is because, with the recently introduced support for line-continuation markers in %if expressions (commit 5f4fdce), the parser would be tricked into thinking that the markers belong to the %if expression itself and collapse them into a single line, including the matching %endif, and then complain about the missing %endif (which must be on a separate line). Instead, similarly to %if, we should collapse the %define/%global macro itself so that the line parser doesn't pass through the body at all. A side effect of this change (of commit 5f4fdce from a year ago, in fact) is that %if constructs are no longer syntax-checked within macros defined in false %if branches, but that's arguably correct behavior, since they only come into existence when their definition macro (%define or %global) is actually expanded, and should merely be treated as text values until that point. Fixes: rpm-software-management#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/warn otherwise. How easy (or not) that is to achieve, I don't know. The thing is, this is only ambiguous within a spec (or an %included snippet). Anywhere else it is just arbitrary text which the macro engine will pass through as-is. So it should only be an error/warning when used from a %define or %global from inside a spec. |
|
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: This produces an empty description in the package, no warnings, and removing the %description above the %test line produces identical output 🤪 |
|
Yes, this is really ugly :) It turns out, though, these 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. |
|
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. 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. 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 it seems the only thing needed would be adding a warning about ambiguous %if and friends when not escaped, somehow. I could be missing details here, though. |
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. If you would "collapse" the 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 |
|
Basically the closest example from the C language would be |
Since the body of a newly defined macro may span multiple lines and contain %if expressions, we need to make sure the line parser does not try to interpret those when the corresponding %define or %global macro appears in a false %if branch and is therefore left unexpanded in the line buffer. This is usually not a problem since any macros found in the body itself would not be expanded anyway, but it can break the syntax check for conditionals, which follows after expansion. This is because, with the recently introduced support for line-continuation markers in %if expressions (commit 5f4fdce), the parser would be tricked into thinking that the markers belong to the %if expression itself and collapse them into a single line, including the matching %endif, and then complain about the missing %endif (which must be on a separate line). Instead, similarly to %if, we should collapse the %define/%global macro itself so that the line parser doesn't pass through the body at all. A side effect of this change (of commit 5f4fdce from a year ago, in fact) is that %if constructs are no longer syntax-checked within macros defined in false %if branches, but that's arguably correct behavior, since they only come into existence when their definition macro (%define or %global) is actually expanded, and should merely be treated as text values until that point. Fixes: rpm-software-management#1198
|
I'm actually looking for a way to use conditionals in a macro definition file. Do I understand correctly that escaping the conditionals will work with both 4.14 and 4.15 (and even after future changes that add a warning if used unescaped) and produce the intended result? I currently have something like this in mind: %bar \
%%if 0%{!?foo:1} \
val1 \
%%else \
val2 \
%%endif \
%{nil}(And, for that matter, would I need to actually escape |
Don't. Or rather, use expressions which are a macro native construct and do not depend on crazy interactions with the spec parser. |
|
01.06.2022 14:36, Panu Matilainen пишет:
I'm actually looking for a way to use conditionals in a macro definition file.
Don't. Or rather, use expressions which are a macro native construct and do not depend on crazy interactions with the spec parser.
Or write some crazy code using lua :)) golang-rpm-macros are a good example that show many aspects of how lua can be used :))
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Make sure that
foois not defined in your environment.This produces:
This is happening with
rpm-build-4.15.90-0.git14971.8.fc33.x86_64.The text was updated successfully, but these errors were encountered: