-
Notifications
You must be signed in to change notification settings - Fork 360
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
base: master
Are you sure you want to change the base?
Conversation
7bdc64e
to
0bf2ff8
Compare
da62a75
to
9e3db2c
Compare
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. |
Dropped that dependency. |
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. |
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? |
@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. |
I checked how quotes in strings are handled correctly with the changes from this pull request:
The result is that the parameter But the same issue also happens with the present macros related to returning flags and/or values
so there is no difference here. |
Since
which gave the expected results (apart from adding an extra space between the flag and the value). Running
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. |
There is already a function named 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
Isn't that $ 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] |
With the new ME_QUOTED support added end of last year we could make this work correctly. So the question is if we want |
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. |
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. |
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). |
(We might be able to change the behavior of %-f so that it includes all occurrences, but even that makes me a bit uneasy.) |
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. |
Just realized %{-f} and %{-f*} explictly documented as being the last occurence. So yep, we can't change that. |
But the change that documented it was commit a5bd757 by Ralf done March 2023.... |
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. |
Speaking of $ 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 |
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:
|
Fix #546