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

Macro arguments do not support quoting #222

Closed
pmatilai opened this issue May 23, 2017 · 14 comments
Closed

Macro arguments do not support quoting #222

pmatilai opened this issue May 23, 2017 · 14 comments
Assignees
Milestone

Comments

@pmatilai
Copy link
Member

Originally came up while discussing #217:
Especially now that rpm is expanding macro arguments, it needs a way to quote the arguments to handle whitespace, empty arguments etc.

This matches what shell does when undefined arguments are passed to functions:

rpm --define "%foo() 1:%1 2:%2" --eval "%foo %nil bar"
1:bar 2:%2

In some cases that's exactly what's wanted, in some cases it absolutely is not - what the shell has and we don't is a way to quote the arguments to preserve empty ones etc. We need that too, and I think this is a blocker for 4.14 release.

Teaching argvSplitString() to handle quotes (optimally when a flag is enabled to preserve compatibility) would serve various other places too. And if there's a way to quote, there needs to be a way to escape those quotes to pass literal quotes.

@pmatilai pmatilai added this to the rpm-4.14.0 milestone May 23, 2017
@pavlinamv pavlinamv self-assigned this Jun 1, 2017
ffesti pushed a commit that referenced this issue Jun 19, 2017
With double quoting, macro arguments may contain spaces.

Every odd occurrence of double quotes denotes begin of an argument.
Every even occurrence denotes the end of an argument.
Arguments are taken without quotes.

Examples:
./rpm --define "%foo() 1:%1 2:%2 3:%3" \
         --eval '%foo "Next argument will be empty" "" "Last argument"'
1:Next argument will be empty 2: 3:Last argument

or without spaces between arguments:

./rpm --define "%foo() 1:%1 2:%2 3:%3" \
         --eval '%foo "Next argument will be empty""""Last argument"'
1:Next argument will be empty 2: 3:Last argument
ffesti pushed a commit that referenced this issue Jun 19, 2017
With single + double quotes, macro arguments may contain spaces,
and quotes in arguments. The empty argument is "" or ''.

Quotes work similarly as in bash. The only difference is that meaning
of single and double quotes is the same.
@ignatenkobrain
Copy link
Contributor

[brain@ignatenko-w541 kojilogs]$ ~/Projects/upstream/rpm/rpm --define "%foo() 1:%1 2:%2" --eval "%foo %nil bar"
1:bar 2:%2

@pmatilai
Copy link
Member Author

pmatilai commented Aug 4, 2017

@ignatenkobrain, that's the unquoted case. What we're interested about here is:

rpm --define "%foo() 1:%1 2:%2" --eval "%foo '%nil' bar"

...which should return "1: 2:bar". This is not resolved yet, the patches were reverted in commit fe7cf85 as it wasn't ready yet. Reopening.

@pmatilai pmatilai reopened this Aug 4, 2017
@ignatenkobrain
Copy link
Contributor

@pmatilai ohhh, sorry then

pmatilai pushed a commit that referenced this issue Aug 9, 2017
Single or double quotes can be used in macro arguments
as expected - they denote the begin and the end of the argument.

Example with 2-nd and 3-rd argument in double quotes:
./rpm --define "%foo() 1:%1 2:%2 3:%3" \
         --eval '%foo Next_argument_will_be_empty "" "Last argument"'
1:Next_argument_will_be_empty 2: 3:Last argument

Example with all arguments in single quotes,
without spaces between arguments:

./rpm --define "%foo() 1:%1 2:%2 3:%3" \
         --eval "%foo 'Next argument will be empty''''Last argument'"
1:Next argument will be empty 2: 3:Last argument
@pmatilai
Copy link
Member Author

pmatilai commented Aug 9, 2017

Implemented in commit 47f0a89, thanks Pavlina.

@pmatilai pmatilai closed this as completed Aug 9, 2017
@pmatilai
Copy link
Member Author

So... this is not going to fly, too many macros rely on quotes passing untouched (and why shouldn't they, that's the way its been all this time). We'll need some other kind of solution that's opt-in one way or the other. Reopening.

@pmatilai pmatilai reopened this Aug 14, 2017
@pmatilai
Copy link
Member Author

@mlschroe and others too of course: ideas welcome...

@pmatilai
Copy link
Member Author

pmatilai added a commit that referenced this issue Aug 14, 2017
This simply breaks too many things - whole macro ecosystems exist based
on the assumption that quotes in arguments will pass to macros
untouched. Macro argument quoting support is necessary but it'll
need some entirely different approach that is either opt-in or
based on a different syntax altogether.

This reverts commit 47f0a89.
pmatilai added a commit that referenced this issue Aug 14, 2017
This simply breaks too many things - whole macro ecosystems exist based
on the assumption that quotes in arguments will pass to macros
untouched. Macro argument quoting support is necessary but it'll
need some entirely different approach that is either opt-in or
based on a different syntax altogether.

This reverts commit 47f0a89.

(cherry picked from commit a3c1f73)
@n3npq
Copy link
Contributor

n3npq commented Aug 14, 2017

Leave bad enough alone imho.

Every macro templating language in the world has quoting problems: its the nature of the beast.

And a Principle of Least Surprise "Do what bash does!" sets expectations that are wildly out of line with reality.

FWIW, the only place that macros MUST be used (instead of bash/lua/etc) is in the preamble and in %files. Use whatever language you wish to program in, and use macros for templating, not programming.

RPM macros are a gawd awful way to attempt to solve programming issues ...

@proyvind
Copy link
Contributor

Lack of quoting support was actually what made me put the python interpreter support implemented @rpm5.org to use, which in turn made templating a shitload easier with a language which supports overloading of parameters, OOP etc.
Lack of support for such in lua (combined with not exactly widespread knowledge of the language amongst packagers) is why it's not suitable for the task, hence #190 (which again makes templating fully possible for whatever plugin that implements the rpminterp.h api, that can be implemented and maintained by third parties for ie. turning existing package/spec generators into superiour single line spec files requiring minimal package maintenance);)

pmatilai added a commit that referenced this issue Aug 31, 2017
All the nice quote-characters are already spoken for, we need to
do something more special here. Add a special-purpose built-in %{quote:...}
macro which quotes its argument using ASCII unit separator character 0x1f
(so it really shouldn't get into anybodys way) and teach macro argument
splitting to support that.

So with %{quote:...} it's now possible to pass strings containing
whitespace and empty strings as arguments. It might not be pretty, but
it's at least POSSIBLE, and no existing user is bothered by this.
@pmatilai
Copy link
Member Author

Since there are no nice quote characters left, implemented with the help of a special %{quote:...} macro. It might not be the prettiest solution in existence but at least it makes it possible to quote the arguments without stepping on any existing uses I'd hope.

pmatilai added a commit that referenced this issue Aug 31, 2017
All the nice quote-characters are already spoken for, we need to
do something more special here. Add a special-purpose built-in %{quote:...}
macro which quotes its argument using ASCII unit separator character 0x1f
(so it really shouldn't get into anybodys way) and teach macro argument
splitting to support that.

So with %{quote:...} it's now possible to pass strings containing
whitespace and empty strings as arguments. It might not be pretty, but
it's at least POSSIBLE, and no existing user is bothered by this.

(cherry picked from commit cdbc7e1)
ydario pushed a commit to bitwiseworks/rpm that referenced this issue Oct 21, 2017
Single or double quotes can be used in macro arguments
as expected - they denote the begin and the end of the argument.

Example with 2-nd and 3-rd argument in double quotes:
./rpm --define "%foo() 1:%1 2:%2 3:%3" \
         --eval '%foo Next_argument_will_be_empty "" "Last argument"'
1:Next_argument_will_be_empty 2: 3:Last argument

Example with all arguments in single quotes,
without spaces between arguments:

./rpm --define "%foo() 1:%1 2:%2 3:%3" \
         --eval "%foo 'Next argument will be empty''''Last argument'"
1:Next argument will be empty 2: 3:Last argument
ydario pushed a commit to bitwiseworks/rpm that referenced this issue Oct 21, 2017
…)"

This simply breaks too many things - whole macro ecosystems exist based
on the assumption that quotes in arguments will pass to macros
untouched. Macro argument quoting support is necessary but it'll
need some entirely different approach that is either opt-in or
based on a different syntax altogether.

This reverts commit 47f0a89.
ydario pushed a commit to bitwiseworks/rpm that referenced this issue Oct 21, 2017
…anagement#222)

All the nice quote-characters are already spoken for, we need to
do something more special here. Add a special-purpose built-in %{quote:...}
macro which quotes its argument using ASCII unit separator character 0x1f
(so it really shouldn't get into anybodys way) and teach macro argument
splitting to support that.

So with %{quote:...} it's now possible to pass strings containing
whitespace and empty strings as arguments. It might not be pretty, but
it's at least POSSIBLE, and no existing user is bothered by this.
@jnpkrn
Copy link
Contributor

jnpkrn commented Aug 2, 2018

Looks like a perfect timing regarding discovery and fixing
of silent issues in one of my specfiles:

%py_byte_compile '%{__python3} \-I' '%{buildroot}%{_datarootdir}/%{name}/ext-plugins'

Why nobody told me that quotes are taken literally and only spaces/tabs
play the role in word/argument splitting, nor any warning/error about
argument number mismatch was raised?! As you can, see, I was already
forced to escape the dash, which was originally indicating this true
reality, but I managed to silence it by force :-)

Luckily, the problem was exposed recently thanks to this change:
https://src.fedoraproject.org/rpms/python-rpm-macros/c/05333eb5866a865282e37ad4d8d24f0b2b6cabb1?branch=master#diff-file-1
and thanks to %1 not being shell-quoted at line 18, which resulted in
an invalid bash syntax (unexpected EOF while looking for matching `'')
because of:

python_version=$('/usr/bin/python3 -c "import sys; ...

So I figure %{quote:...} is likely the proper solution here,
thanks for that!

However!
Is there any trick that would make using that construct at least
partially compatible with older rpm versions, e.g. conditionalizing
its usage somehow? If not, I'd strongly suggest it'd be considered
prior to rpm-4.14.2-release so that mutual compatibility can be
achieved (in my case, for instance, it would be using merely python
path without additional switches when explicit quoting not supported)!

EDIT with possible solution:

rpm-4.14.2-0.rc1.2.fc29.x86_64:
$ rpm -E '%defined quote'
10

rpm-4.13.0.1-7.fc26.x86_64:
$ rpm -E '%defined quote'
0

EDIT 2 and what seems to be working:

%if "%{?quote:1}" != "" && "%{?quote:1}" != "1"
%py_byte_compile %{quote:%{__python3} -I} %{buildroot}%{_datarootdir}/%{name}/ext-plugins
%else
%py_byte_compile %{__python3} %{buildroot}%{_datarootdir}/%{name}/ext-plugins
%endif

@n3npq
Copy link
Contributor

n3npq commented Aug 2, 2018

RPM macros are context free: expanded within quotes, within comments, everywhere a % is found.

Always been that way.

@n3npq
Copy link
Contributor

n3npq commented Aug 2, 2018

There are 2 issues that would need to be solved if you wish portability within versions of rpm macro expansion:

  • adding -I to the Python interpreter in use
  • handling a possible space in the file name

There are means to solve both of those issues by not using parameterized macros (because it's unclear whether -I is a parameterized macro or a Python option) and by elimininating the usage of position sensitive %1 (by expanding within single quotes before executing Python).

YMMV, everyone's does. Retrofitting macro context within single quotes (or the very ugly %{quote: ...}, or changing behavior to expand arguments before passing to a parameterized macro, is almost certainly going to have a legacy code behavior debt that cannot be solved adequately.

@n3npq
Copy link
Contributor

n3npq commented Aug 3, 2018

(after looking at recent changes to macro expansion)

Parameterized macros use posix getopt(3) for parsing macro options/arguments, and getopt is perfectly capable of honoring quoting similar to bash.

Sadly the implementation in rpm does not use getopt argv but attempts its own parser in order to expand each macro argument individually, honoring not only quoting, but also %(...) diversions, %{...} macro scoping, and (most painful of all to get right for positional args) empty arguments that expand to "".

The parser in rpmio/macro.c needs fixing to handle quoting (without the ugly %{quote: ...} construct), as well as the possibility that a single parameterized macro argument may lead to multiple arguments to pass to getopt(3) some of which may need quoting (because empty or having white space embedded) before being executed.

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

No branches or pull requests

6 participants