Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Triple operator for conditional shortcut #115

Closed
ascherer opened this issue Jan 4, 2017 · 22 comments
Closed

Triple operator for conditional shortcut #115

ascherer opened this issue Jan 4, 2017 · 22 comments
Labels

Comments

@ascherer
Copy link
Contributor

ascherer commented Jan 4, 2017

I suggest the following notations

%{?condition:true!false}
%{!?condition:false!true}

as shortcuts for the current form

%{?condition:true}%{!?condition:false}
@pmatilai pmatilai added the RFE label Feb 21, 2017
@pmatilai
Copy link
Member

Yup, a ternary operator for macro conditionals would be really handy. No strong opinion on the separator character. Other than noting how unfortunate it is that rpm used a syntax that is incompatible with the rest of the world :(

@pavlinamv
Copy link
Contributor

The separator character should be chosen such that it will not cause problems with old macros:
%{?condition:true}
or
%{!?condition:false}

If we define, that:
separator operator is the first '!' after ':', which is not nested in {} or in ()
it will change interpretation of macros like:
%{?write_errors: You did not respond! It is a mistake.}

@pavlinamv
Copy link
Contributor

I do not see any possibility how to define sensible syntax of the triple operator, without possible causing problems for macros %{?condition:true} and %{!?condition:false}. Thus I think that adding this macro without additional changes is not a good idea.

The syntax of the macro should start "%{?condition:" or "%{!?condition:"
But then there is a problem with next separator operator:
E.g if we define syntax
%{?condition:true!false},
where separator operator is the first '!' after ':', which is not nested in {} or in ()
it will change interpretation of macros like
%{?write_errors: You did not respond! It is a mistake.}
which will change from its original meaning
if "write_errors" is defined, then write "You did not respond! It is a mistake."
to
if "write_errors" is defined, then write "You did not respond" else write " It is a mistake."

If we change separator operator from "!" to another character it will not help us a lot. The mentioned type of mistake will still occur. The only characters which are different from rpm point of view are "{", "}", "(", ")". To use some of them is not a good choice too. The triple operator will look very weird and probably another type of problems occur.

Similarly for a separator operator defined as a sequence of chars.

@pavlinamv
Copy link
Contributor

Thinking about it some more... Syntax:

%{?condition:{true}!{false}}
%{!?condition:{false}!{true}}

is OK. So if it is acceptable, I will make a patch for it.

@pmatilai
Copy link
Member

An old macro could just as legally and likely contain standalone { } characters as it can contain !'es, I don't see that as being any safer really.

@pavlinamv
Copy link
Contributor

Notation

%{?{condition}:true:false}
%{!?{condition}:false:true}

looks promising for me.

  1. It is because it causes no problems in old macros.
  2. It looks quite naturaly, the only difference from the most expected notation are curly baces around the condition. (They are added to reach 1).

Similar notations that can be chosen instead:

%{?{condition}:true!false}
%{?{condition}?true!false}
%{?{condition}?true:false}
..

More detailed description of the notation:

  • Between the first chars "%{?" resp. "%{!?" or "%{?!" there can not be a space.
  • On the other hand arround {condition} and :, there can but not need to be a space.
  • In {condition}, there can be a space before or after condition.
    It should be highly recommended to use %{true} and %{false} instead of true or false.

It can not cause problems in the existing macros because:
If curly brackets around condition occure in the existing spec file, the current implementation resolves
%{?{condition} .... } to "" (all cases without the space between '?' and '{' are resolved to "")
and
%{? {condition} .... } resolves to "%".
E.g. %{!?{-n}:%{1}}%{?{-n}:%{-n*}} expands in all cases to "".
It means that the spec behaves differently than the author expected.

If the implementation will be done wisely it can change the expansion to more
natural case:
%{!?{-n}:%{1}}%{?{-n}:%{-n*}} -> expand to %{1} or %{-n*} according to "-n".

The only different unexpected beaviour for the current spec files can arise in a case like: %{!?{condition}:sth1:sth2}. In such a case the current implementation expands it to "" and the new behaviour will expand it to sth1 or sth2. Thus it can potentially change behavior from the current one to different unexpected behavior. But such case is highly unlikely and the original behavior is already different from the expected one (as explained above). So it changes one unexpected behavior to some different one.

@mlschroe
Copy link
Contributor

mlschroe commented Dec 7, 2018

Bike shedding: I'd prefer %{?:condition:true:false} and %{?!:condition:false:true} which also should not conflict with current macro usage.

Or %{?condition?true:false} if we're sure that ? cannot be in a macro name.

@pavlinamv
Copy link
Contributor

You are correct %{?:condition:true:false} and %{?!:condition:false:true} also should not conflict with the current macro usage. I am OK with this notation too.

@voxik
Copy link
Contributor

voxik commented Jan 3, 2019

Given the proposed syntax, which looks ugly in all cases, I'd prefer to forget about the ternary operator in RPM and focus on other important issues of RPM.

pavlinamv added a commit to pavlinamv/rpm that referenced this issue Jun 12, 2019
…ment#115)

%{?  { macro_name } : true  : false }
%{?  { macro_name } : true          }
%{?! { macro_name } : false : true  }
%{?! { macro_name } : false         }

More detailed description of the notation:
* Between the first chars "%{?" resp. "%{!?" or "%{?!"
  there can not be a space.
* On the other hand arround {condition} and :, there
  can but not need to be a space.
* In {condition}, there can be a space before or after the
  macro_name.

So e.g. the folowing conditions are similar (from rpm
point of view):

%{?{macro}:true:false}
%{? { macro } : true : false }

The relevant tests are added.
pavlinamv added a commit to pavlinamv/rpm that referenced this issue Jun 12, 2019
%{?  { macro_name } : true  : false }
%{?  { macro_name } : true          }
%{?! { macro_name } : false : true  }
%{?! { macro_name } : false         }

More detailed description of the notation:
* Between the first chars "%{?" resp. "%{!?" or "%{?!"
  there can not be a space.
* On the other hand around {condition} and :, there
  can but not need to be a space.
* In {condition}, there can be a space before or after the
  macro_name.

So e.g. the following conditions are similar (from rpm
point of view):

%{?{macro}:true:false}
%{? { macro } : true : false }

The relevant tests are added.

Closes: rpm-software-management#115
pavlinamv added a commit to pavlinamv/rpm that referenced this issue Jul 9, 2019
%{?  { macro_name } : true  : false }
%{?  { macro_name } : true          }
%{?! { macro_name } : false : true  }
%{?! { macro_name } : false         }

More detailed description of the notation:
* Between the first chars "%{?" resp. "%{!?" or "%{?!"
  there can not be a space.
* On the other hand around {condition} and :, there
  can but not need to be a space.
* In {condition}, there can be a space before or after the
  macro_name.

So e.g. the following conditions are similar (from rpm
point of view):

%{?{macro}:true:false}
%{? { macro } : true : false }

The relevant tests are added.

Closes: rpm-software-management#115
pavlinamv added a commit to pavlinamv/rpm that referenced this issue Aug 5, 2019
%{?  { macro_name } : true  : false }
%{?  { macro_name } : true          }
%{?! { macro_name } : false : true  }
%{?! { macro_name } : false         }

More detailed description of the notation:
* Between the first chars "%{?" resp. "%{!?" or "%{?!"
  there can not be a space.
* On the other hand around {condition} and :, there
  can but not need to be a space.
* In {condition}, there can be a space before or after the
  macro_name.

So e.g. the following conditions are similar (from rpm
point of view):

%{?{macro}:true:false}
%{? { macro } : true : false }

The relevant tests are added.

Closes: rpm-software-management#115
@pmatilai
Copy link
Member

pmatilai commented Aug 6, 2019

While we're thinking about extending the conditional macro syntax, here's another thing to consider:

The current ? test is only for (non-)existence of macro, which is extremely limiting. We could easily make the spec %if expression parser available to macro engine, which would give a whole new level of power to macros. I think we should plan the new syntax to be able to accommodate this, even if it's not initially implemented, we don't want to have to invent yet another syntax for that later on.

pavlinamv added a commit to pavlinamv/rpm that referenced this issue Aug 6, 2019
%{?{macro_name}:true:false}
%{?{macro_name}:true}
%{?!{macro_name}:false:true}
%{?!{macro_name}:false}

The relevant tests are added.

Closes: rpm-software-management#115
pavlinamv added a commit to pavlinamv/rpm that referenced this issue Aug 6, 2019
Accepted syntax:
%{?{macro_name}:true:false}
%{?{macro_name}:true}
%{?!{macro_name}:false:true}
%{?!{macro_name}:false}

The relevant tests are added.

Closes: rpm-software-management#115
@pmatilai
Copy link
Member

Another thing is that this syntax makes it impossible to have colons (':') in the output (eg '%{!?foo::}'), which is a limitation the original syntax doesn't have'. It obviously has it's own set of limitations and quirks...

@pmatilai
Copy link
Member

PR #817 adds support for arbitrary expression parsing in macros. The real power would come from wedding that to macro conditionals, and that's the thing I want to see at least planned for before we add any new condition syntaxes.

@pavlinamv
Copy link
Contributor

Another thing is that this syntax makes it impossible to have colons (':') in the output (eg '%{!?foo::}'), which is a limitation the original syntax doesn't have'. It obviously has it's own set of limitations and quirks...

Not impossible, but not straightforward. For ':' in the output you can use %define / %global:
rpm --define "text1 :" --eval '%{!?{foo}:%{text1}:%{text2}}'
or you can use %expand:
rpm --eval '%{!?{foo}:%{expand::}:%{expand::}}

pavlinamv added a commit to pavlinamv/rpm that referenced this issue Aug 15, 2019
Accepted syntax:
%{?{macro_name}:true:false}
%{?{macro_name}:true}
%{?!{macro_name}:false:true}
%{?!{macro_name}:false}

The relevant tests are added.

Closes: rpm-software-management#115
@pavlinamv
Copy link
Contributor

While we're thinking about extending the conditional macro syntax, here's another thing to consider:

The current ? test is only for (non-)existence of macro, which is extremely limiting. We could easily make the spec %if expression parser available to macro engine, which would give a whole new level of power to macros.

Please, what it the expected be gain from it?

@pmatilai
Copy link
Member

I thought the potential gains from ability of testing arbitrary expressions instead of simple macro existence would be obvious enough not to need explanations.

The most basic case is that there's tonne of functionality in rpm which uses macro existence test to determine whether something is enabled or disabled, because that's all that's available. When people see something like "%_include_minidebuginfo 1", they tend to assume defining it to 0 disables it, and get confused and annoyed when it doesn't. It gets worse because, as you know, macros stack so undefining doesn't guarantee said macro actually goes away. So in practise, there's no guaranteed way to disable such a feature from a spec or otherwise.

There are countless other examples, such as defining a macro depending on the string value of another, etc.

@pmatilai
Copy link
Member

pmatilai commented Aug 22, 2019

To elaborate a bit further... so what rpm really needs, much more than the triple-operator for existence, is a macro syntax that supports the generic form:

<expression> ? <true case> [: <false case> ] 

expression is an arbitrary expression evaluated by rpmExprBool() (ie the same as spec %if), true case is output when expression is true, optionally followed by a false case. The macro existence test (triple syntax or not) could be seen as a specific sub-case of this all.

@mlschroe
Copy link
Contributor

Just to start the discussion, I wonder if support for %{expr:<expr>?<true>} and
%{expr:<expr>?<true>:<false>} is too insane? If <expr>/<true>/<false> contains a '?' or ':' char you could use %{quote:} as workaround. E.g.:

%{expr:%_include_minidebuginfo?%{quote:mini:true}:%{quote:mini:false}}

@mlschroe
Copy link
Contributor

OTOH we could add a ? : operator to rpm's expression parser:

%{expr:%_include_minidebuginfo?"mini:true":"mini:false"}

@pmatilai
Copy link
Member

My general idea has been that %{expr:...} is strictly for evaluating expressions into strings, and that a different syntax would be used for boolean evaluation, something along the lines of
%{(<expr>)?<true>:<false>} but haven't given the syntax details too much thought.

Adding support for ? :in the expression parser is certainly a possibility as well.

@mlschroe
Copy link
Contributor

Support for ? :in the expression parser has the addition bonus that it resolves the "whitespace stripping" discussion:

%{expr: 0%?_include_minidebuginfo ? "mini:true" : "mini:false" }

Also note that the expr parser already has a (somewhat insane) macro expansion feature:

%{expr: ix86}

@pavlinamv
Copy link
Contributor

I do not see any problem in the syntax that @pmatilai proposed in his previous comment:
%{(<expr>)?<true>:<false>}
There are two similar options that are closer to the currently proposed triple condition operator syntax (#746):
%{{<expr>}?<true>:<false>}
%{{<expr>}:<true>:<false>}

Using these syntax the example from the previous @mlschroe comment looks:
%{(0%?_include_minidebuginfo)?"mini:true":"mini:false"}
%{{0%?_include_minidebuginfo}?"mini:true":"mini:false"}
%{{0%?_include_minidebuginfo}:"mini:true":"mini:false"}

Syntax:
%{expr: 0%?_include_minidebuginfo ? "mini:true" : "mini:false" }
can cause a small problem in deciding which ? divides the parts of the macro. But it can be solved - the first ? can be in {}:
%{expr:0%{?_include_minidebuginfo}? "mini:true" : "mini:false"}

@pmatilai
Copy link
Member

pmatilai commented Sep 23, 2019

So thanks to @mlschroe , we now have much more than originally bargained for. See https://github.com/rpm-software-management/rpm/blob/master/doc/manual/macros#L238 for docs and from

AT_SETUP([expr macro 1])
onwards for practical examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants