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

Add macro '%-x**' containing all occurrences of the flag '-x' #2449

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rhabacker
Copy link
Contributor

@rhabacker rhabacker commented Mar 23, 2023

Fix #546

@rhabacker rhabacker force-pushed the fix-546 branch 6 times, most recently from 7bdc64e to 0bf2ff8 Compare March 24, 2023 09:55
@rhabacker rhabacker changed the title Add macro '-x**' containing all occurrences of the flag '-x' Add macro '%-x**' containing all occurrences of the flag '-x' Mar 24, 2023
@rhabacker rhabacker force-pushed the fix-546 branch 4 times, most recently from da62a75 to 9e3db2c Compare March 24, 2023 15:27
@rhabacker
Copy link
Contributor Author

This pull request required several push forces because the master branch of rpm on openSUSE 15.4 could not be built due to the missing package rpm-sequoia. The patch was therefore developed on rpm-4.14.x and ported to the master branch.

@rhabacker
Copy link
Contributor Author

depends on #2455

Dropped that dependency.

@rhabacker
Copy link
Contributor Author

Is there anything else that would block the merge? I need this feature to fix a problem I am having on https://build.opensuse.org/project/show/windows:mingw:win32.

@rhabacker
Copy link
Contributor Author

Is there anything else that would block the merge? I need this feature to fix a problem I am having on https://build.opensuse.org/project/show/windows:mingw:win32.

The dbus project has reported a problem in this regard on https://gitlab.freedesktop.org/dbus/dbus/-/issues/455 for which this support is required, since the associated macro mingwxx-cmake must be adapted to filter out all occurrences of -Dxxx and --xxx together with -B and -S.

Alternatively, this functionality would have to be (re)implemented as a lua macro, which in my opinion is a waste of time, since there is already a solution with this merge request.

@pmatilai
Copy link
Member

Sorry for not responding earlier, we've been busy with 4.19 alpha preparations and stuff.

The question is whether this is the way we want to address this particular issue. Separating the arguments by space introduces a new problem as arguments can legitimately have spaces in them (by using %{quote:...}) and we don't want to introduce a feature that's not compatible with the rest of the system. @mlschroe , thoughts?

@DemiMarie
Copy link
Contributor

@pmatilai what about quoting each argument separately, or making them available as a Lua array? IMO any macro this complex should probably be written in Lua.

@rhabacker
Copy link
Contributor Author

@pmatilai what about quoting each argument separately,

I checked how quotes in strings are handled correctly with the changes from this pull request:

$ ../rpm-build/rpm --define '%foo(D:) %{-D**}' --eval '%foo -D11  -D 22 -D"33" -D"44 55" -D "66 77"    argument'
 -D11 -D 22 -D"33" -D"44 -D "66

The result is that the parameter -D"44 55" and -D "66 77" are not recognized correctly.

But the same issue also happens with the present macros related to returning flags and/or values

$ ../rpm-build/rpm --define '%foo(D:) %{-D}' --eval '%foo -D"44 55"   argument'
-D"44

../rpm-build/rpm --define '%foo(D:) %{-D*}' --eval '%foo -D "44 55"   argument'
"44

so there is no difference here.

@rhabacker
Copy link
Contributor Author

Since rpm uses a call to getopt to process the command line, I checked the parameter list with the command line tool getopt.

$ getopt -o "D:" -- -D11 -D 22 -D "33" -D "44 55" -D "66 77"
 -D '11' -D '22' -D '33' -D '44 55' -D '66 77' --

which gave the expected results (apart from adding an extra space between the flag and the value).

Running getopt with gdb and inspecting the argv array

Breakpoint 1, main (argc=11, argv=0x7fffffd7d8) at misc-utils/getopt.c:369
369 struct getopt_control ctl = {
(gdb) p argv[8]
$10 = 0x7fffffdd66 "-D44 55"
(gdb) p argv[9]
$11 = 0x7fffffdd6e "-D"
(gdb) p argv[10]
$12 = 0x7fffffdd71 "66 77"
(gdb)

shows that the quoting is done by the shell and not by getopt itself. The conclusion is that something similar must be done before calling getopt in rpm to process macro parameters with quotes.

@rhabacker
Copy link
Contributor Author

... The conclusion is that something similar must be done before calling getopt in rpm to process macro parameters with quotes.

There is already a function named splitQuoted(), which splits the arguments, currently only separated by space and tabs.

It looks like a candidate for processing quotes.

… '-x <arg>'.

Flags are added to this macro with the flag and argument
in the original notation and separated from previous by spaces.

Fix rpm-software-management#546
@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 22, 2024

@rhabacker

shows that the quoting is done by the shell and not by getopt itself. The conclusion is that something similar must be done before calling getopt in rpm to process macro parameters with quotes.

Isn't that %quote, like @pmatilai mentioned? Going back to your experiments (braces added to output for clarity):

$ rpm --define '%foo(D:) [%{-D}]' --eval '%foo -D%{quote:44 55} argument' 
[-D 44 55]
$ rpm --define '%foo(D:) [%{-D*}]' --eval '%foo -D%{quote:44 55} argument'
[44 55]

@mlschroe
Copy link
Contributor

With the new ME_QUOTED support added end of last year we could make this work correctly. So the question is if we want %-x** or not.

@mlschroe
Copy link
Contributor

I don't think %-x** should repeat the option name, though. It should just be the arguments, i.e. just like %-x* but with multiple values.

@pmatilai
Copy link
Member

Like I said before, I'm not convinced this is the way we want to represent the multivalue case.

Assuming we can technically do this now, is there a reason not to put those values into the normal -x* macro? It seems to me we can't really break any significant existing users as we just haven't supported multiple flags before, so macros can't have been using them much.

And, I concur with @DemiMarie here - I tend to think this would be better left to Lua where an array of values can be properly dealt with.

@mlschroe
Copy link
Contributor

Historically it has always been just the last occurrence, so I don't think we can change this.

Regarding the array of values part: we now can deal properly with it because of ME_QUOTED. But I'm fine if the functionality is only available to lua (but it currently is not).

@mlschroe
Copy link
Contributor

(We might be able to change the behavior of %-f so that it includes all occurrences, but even that makes me a bit uneasy.)

@pmatilai
Copy link
Member

pmatilai commented Jan 22, 2024

I know its that way historically, just doubtful of people actually relying on multivalues being handled that way. But then, I wouldn't know. There's some risk involved for sure.

Yet another possibility could be letting macros declare the way they want their arguments, ie "I can handle multiple values, bring em on", just like we have a way to disable all pre-processing.

@pmatilai
Copy link
Member

Just realized %{-f} and %{-f*} explictly documented as being the last occurence. So yep, we can't change that.

@mlschroe
Copy link
Contributor

But the change that documented it was commit a5bd757 by Ralf done March 2023....

@pmatilai
Copy link
Member

Actually, there is a kind of a way to access the values from Lua even now.

%-f* macro has the last definition of the macro. If you pop (ie undefine) it, you get the one underneath. And with that, you can walk all the values. It wont work on normal macros as "%undefine -f*" is not permitted, but rpm.undefine() in Lua bypasses that check.

Not that this is something I'd recommend for use.

@pmatilai pmatilai self-assigned this Jan 24, 2024
@ferdnyc
Copy link
Contributor

ferdnyc commented Jan 25, 2024

With the new ME_QUOTED support added end of last year we could make this work correctly. So the question is if we want %-x** or not.

Speaking of ME_QUOTED: On a somewhat related note, am I doing something wrong or does %{quote} seem to not play nicely with %{**}?

$ rpm --define '%foo(D:) %{**}' --eval '%foo -D"33 44"  argument' 
-D"33 44" argument

$ rpm --define '%foo(D:) %{**}' --eval '%foo -D%{quote:33 44}  argument'
-D33 44 argument

@mlschroe
Copy link
Contributor

That looks like the correct output to me. Why do you think it doesn't work? Note that %quote does not add any quotes, the effect is purely internal. Here's how to make it visible:

$ rpm --define '%foo(D:) %{shescape %{**}}' --eval '%foo -D"33 44"  argument' 
'-D"33' '44"' 'argument'
$ rpm --define '%foo(D:) %{shescape %{**}}' --eval '%foo -D%{quote:33 44}  argument'
'-D33 44' 'argument'

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

Successfully merging this pull request may close these issues.

RFE: access to multiple instances of the same option
5 participants