[Rpm-maint] [PATCH] forbid #%define

[Panu Matilainen]

On Thu, 5 Jun 2008, Manfred Hollstein wrote:

> On Thu, 05 Jun 2008, 16:36:39 +0200, Pixel wrote:
>> without this patch, "#%define foo bar" is surprisingly equivalent to "%define foo bar"
>> with this patch, "#%define foo bar" is a fatal error.
>
> Hmm, to me this appears like papering over a real problem...
>
>> if you think it's worth it, i can frontport it to git master (this one
>> is for rpm-4.4.x)
>>
>> we could also disable macro expansion in "#%define ....",
>> or in all "#..." (hum, bigger pbs here)?
>
> ... as this appears much more "normal" from a user perspective. When I'm
> writing shell scripts and want to test/debug some areas, I'll just
> comment them by prefixing the stuff with the usual '#' when I'm done.
> Dunno if it's documented anywhere in the RPM documentation that "#...'
> is deprecated/should not be used, but I'd prefer that we fix those
> issues, where a prefixing '#' still results in getting the remainder of
> that line being evaluated...

Nod, the current behavior is certainly not what you expect when you 
comment out a line. It is documented at least here:
http://docs.fedoraproject.org/drafts/rpm-guide-en/ch09s02.html#id2970430 
and http://www.rpm.org/hintskinks/commenting_not_working/ but...

Incidentally this and various related issues wrt spec parsing have come up 
in the last few days from several directions, maybe it's time to take a 
good look at the endless quirks in spec parser and see what could/should 
be done about them. The grand goal would be an actual formal syntax 
specification for specs. I dunno if this possible in the realm of the 
current rather ad-hoc spec syntax, legacy issues and all, but it's at 
least something to strive for.

There's no shortage of things to consider, but since it's macro expansion 
that came up, we might as well start with it.

This sounds like a no-brainer - why should anything be done for comment 
lines? But consider the following spec snippet:

%build
cat << EOF > example.conf
# To enable feature foo, uncomment the following:
#[myplugin]
#pluginpath = %{_libdir}/%{name}/myplugin.so
EOF

In this case, you'd actually want the macros to be expanded, despite being 
apparently being commented out. Of course the comment markers here aren't 
intended for the spec parser at all, but how would the parser know that?
OTOH, if you take something like

%build
<...>
#%define testing 1
%if %{?testing}
cat << EOF > README
This is just a test build
EOF
%endif

...you do NOT want the %define to take effect.

I dunno - one possibility might be requiring use of %{macro} notation 
instead of just %macro to force expansion in comment lines. The macro 
engine doesn't have a notion of "lines", it just operates on text stream 
where # is a character just like any other, this would somehow have to be 
dealt with somehow. A very straightforward and simple rule would be "if 
the first character of the text to process starts is "#", braces are 
required to perform macro expansion"

Simple and stupid quick hack of a patch to do that attached, might not be 
entirely correct for every possible case but seems to do what intended for 
the cases I tested. This would, by the way of spec conventions, take care 
of the typical problem cases like
#%define some thing

If you *really* want that define to take place despite being commented, 
just write
#%{define} some thing

Thoughts?

 	- Panu -
-------------- next part --------------
diff --git a/rpmio/macro.c b/rpmio/macro.c
index b8889ed..1e7c0a2 100644
--- a/rpmio/macro.c
+++ b/rpmio/macro.c
@@ -62,6 +62,7 @@ typedef struct MacroBuf_s {
     int depth;			/*!< Current expansion depth. */
     int macro_trace;		/*!< Pre-print macro to expand? */
     int expand_trace;		/*!< Post-print macro expansion? */
+    int comment;		/*!< "Comment line" mode? */
     void * spec;		/*!< (future) %file expansion info?. */
     rpmMacroContext mc;
 } * MacroBuf;
@@ -1032,6 +1033,7 @@ expandMacro(MacroBuf mb)
     int negate;
     const char * lastc;
     int chkexist;
+    int braces;
 
     if (++mb->depth > max_macro_depth) {
 	rpmlog(RPMLOG_ERR,
@@ -1064,6 +1066,7 @@ expandMacro(MacroBuf mb)
 	if (mb->depth > 1)	/* XXX full expansion for outermost level */
 		t = mb->t;	/* save expansion pointer for printExpand */
 	negate = 0;
+	braces = 0;
 	lastc = NULL;
 	chkexist = 0;
 	switch ((c = *s)) {
@@ -1125,6 +1128,7 @@ expandMacro(MacroBuf mb)
 			rc = 1;
 			continue;
 		}
+		braces = 1;
 		f = s+1;/* skip { */
 		se++;	/* skip } */
 		while (strchr("!?", *f) != NULL) {
@@ -1153,6 +1157,13 @@ expandMacro(MacroBuf mb)
 		break;
 	}
 
+	/* only expand macros in braces in "comment lines" */
+	if (mb->comment && !braces) {
+	    c = '%';	/* save % as we didn't consume it */
+	    SAVECHAR(mb, c);
+	    continue;
+	}
+
 	/* XXX Everything below expects fe > f */
 	fn = (fe - f);
 	gn = (ge - g);
@@ -1382,6 +1393,7 @@ expandMacros(void * spec, rpmMacroContext mc, char * sbuf, size_t slen)
     mb->depth = 0;
     mb->macro_trace = print_macro_trace;
     mb->expand_trace = print_expand_trace;
+    mb->comment = (*(mb->s) == '#');
 
     mb->spec = spec;	/* (future) %file expansion info */
     mb->mc = mc;