Re: [Rpm-maint] [PATCH] Check for undefined macros
On 02/13/2013 04:19 PM, Alexey Tourbin wrote: On Wed, Feb 13, 2013 at 11:52:58AM +0200, Panu Matilainen wrote: With my Fedora maintainer hat on: From rpm POV, Fedora 19 is a done deal at this point, except for the inevitable bugfixes / 4.11.x maintenance release updates. The earliest release where changes of this scale could go in would be rpm 4.12 and Fedora 20 really. I've had enough of playing the we should be able to slip this in just before deadline games by now, new rpm major versions are only introduced in rawhide around the time previous Fedora version has just gotten branched and the schedule for the next one isn't even set yet, and generally needs to be considered stable well before the next branch occurs. So compared to the average Fedora development activities, we're always trying to be roughly three months early (or for alternative POV, one release late) to almost everybody else in landing new developments. When folks are trying to stabilize the rest of the distro, they dont want to (and shouldn't have to) be dealing with new rpm curveballs. The other side is my own sanity (or what's left of it ;) - I just dont want to play the mad and potentially risky dash to deadline when I can largely avoid it by simply aligning schedules a bit differently. It's okay to work toward Fedora 20, although if there were a chance with Fedora 19, I would perhaps try harder to incorporate the patches (not by merely insisting harder, of course). Anyway, I see that there are too many people involved, and so I'm not making a definite judgement, only a suggestion or even a question. Yup, lots of people, policies and bureaucracy involved. Sometimes for to good, sometimes less so... Of course nothing prevents cherry-picking selected bits from git master to, say, Fedora 19 already. Except if I do that, people here complain and want them in an upstream release instead :) From which we get to upstream release policy for rpm.org: maintenance releases should never introduce things that break builds or otherwise cause incompatibilities wrt packages built with the same major branch of rpm. That means specs that are buildable with, say, 4.11.0 need to be buildable with 4.11.5, and the packages built with 4.11.5 must be compatible, autogenerated dependencies included, with packages built with 4.11.0. Additional warnings on suspect syntax etc can however be added. So for example the missing whitespace before macro body patch can pulled into 4.11.x maintenance release as it doesn't change behavior, only warns. On the other hand, the stricter macro substitution syntax patch can not go into 4.11.x (at least as-is) as it actually prevents those buggy specs from building at all. By the way, I was previously involved in a much shorter development cycle: - Hack on rpm and get it into the repo. - Rebuild the whole repo with new rpm (without actually bumping the release/replacing the packages - only for testing purposes). - Compare build logs from previous run, identify and fix regressions. The cycle could be as short as one week, with the whole repo rebuild taking place at the weekend. So all these months and schedules seem a bit odd to me, at least at first blush. :-) I figured this might be quite a culture shock coming from AltLinux background, hence the warning :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH] Check for undefined macros
On Sun, Jan 27, 2013 at 05:27:09AM +, Alexey Tourbin wrote: This change introduces a generalized routine rpmExpandMacros() to expand macros on behalf of user input, with improved diagnostic facilities. This is actually the most important patch, because it unveils many typos/thikos in specfiles, and it can be instrumental in improving package base (as opposed to improving rpm code base proper, with no overt impact on the packages). Since Fedora 19 is in its early stage of development, and since RHEL7 probably won't be using rpm-4.11+ anyway, I wonder whether this is the right time and otherwise a good idea to make a release with this patch applied (along with a few other patches related to syntax). If it turns out to be a good idea, I'm willing to spend more time and come up with full coverage results for Fedora 18 specfiles (I have already posted a rather detailed if preliminary analysis of some common errors). In particular, one of its arguments is a callback function which is called whenever an undefined macro is encountered in user input. When specfile parser calls this routine, it supplies a custom function to handle undefined macros. This function is keenly aware of specfile syntax: valid specfile tokens, such as %prep and %setup, when found in the right context, are not treated as undefined macros. Otherwise, there are 3 possibilities. - Undefined macros in preamble and %pre/%post/... scriptlets raise an error, unless a non-build parse is performed; this is to prevent malformed packages from being built. - In other cases, a warning is issued. - Some tokens are permitted beyond their context (e.g. %ghost in %changelog), to comply with traditional/sloppy usage. Also, undefined macros found in comments normally do not produce a warning. --- build/files.c | 66 ++-- build/pack.c | 11 ++-- build/parseSpec.c | 154 +++--- build/rpmbuild_internal.h | 11 build/spec.c | 3 + rpmio/macro.c | 67 +++- rpmio/rpmmacro.h | 28 ++--- 7 files changed, 287 insertions(+), 53 deletions(-) ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH] Check for undefined macros
On Wed, Feb 06, 2013 at 07:09:00AM +0400, Dmitry V. Levin wrote:
On Sun, Jan 27, 2013 at 05:27:09AM +, Alexey Tourbin wrote:
This change introduces a generalized routine rpmExpandMacros() to expand
macros on behalf of user input, with improved diagnostic facilities.
In particular, one of its arguments is a callback function which is
called whenever an undefined macro is encountered in user input.
When specfile parser calls this routine, it supplies a custom function
to handle undefined macros. This function is keenly aware of specfile
syntax: valid specfile tokens, such as %prep and %setup, when found in
the right context, are not treated as undefined macros. Otherwise,
there are 3 possibilities.
- Undefined macros in preamble and %pre/%post/... scriptlets raise
an error, unless a non-build parse is performed; this is to prevent
malformed packages from being built.
- In other cases, a warning is issued.
- Some tokens are permitted beyond their context (e.g. %ghost in
%changelog), to comply with traditional/sloppy usage. Also, undefined
macros found in comments normally do not produce a warning.
The idea is certainly not new, it was implemented in ALT Linux in October
of 2005, actually. This feature proved to be useful in catching subtle
specfile bugs caused by undefined macros. Said that, one should
The idea is not new, but the implementation has been much refined, and
is now more sensitive to specfile syntax. E.g. it is now possible to
identify subtle bugs like %prep: not starting %prep section properly
(this is actually found in FC18 icon-naming-utils.spec - and I wonder
how they actually managed to build the package). Also, one can actually
subvert the logic in the handler, but this requires some imagination.
For example, one can use something like %{expand:%po}%{expand:st} to
start %post section. Unless deliberate tricks like this are attempted,
I believe the check is pretty robust.
Here is another similar case which would go unnoticed with older
implementation:
$ grep '^%description(' *.spec
trafshow.spec:%description(ru)
$
With new implementation, a warning is issued:
warning: trafshow.spec:31: Undefined macro %description
understand that a change that results to some previously acceptable
specfiles being rejected is a policy enforcement. Whether it is a good
idea to impose this specfile check without some grace period during which
newly recognized specfile errors are reported but not yet raise errors
depends on your user base, percentage of specfiles affected by the change,
how long it would take to fix them all, etc. In ALT Linux, we chosen no
grace period option and introduced %_allow_undefined_macros macro
allowing users to relax the check. Of course, your mileage may vary.
I've posted an analysis of how FC18 specfiles are affected by this change.
It does not include statistics in terms of percents, but it does show that
indeed many bugs can be identified, and also discusses a few cases when
valid packages needs to be adjusted - such as no short-circuit expansion
in conditionals and 'pidof %PPID' in scriptlets.
Finally, I'd like to thank Alexey for bringing this feature to a wider
audience.
I am perhaps left to take reciprocal attitude towards Dmitry and thank
him for first implementing this feature back in 2005.
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [PATCH] Check for undefined macros
On Sun, Jan 27, 2013 at 05:27:09AM +, Alexey Tourbin wrote: This change introduces a generalized routine rpmExpandMacros() to expand macros on behalf of user input, with improved diagnostic facilities. In particular, one of its arguments is a callback function which is called whenever an undefined macro is encountered in user input. When specfile parser calls this routine, it supplies a custom function to handle undefined macros. This function is keenly aware of specfile syntax: valid specfile tokens, such as %prep and %setup, when found in the right context, are not treated as undefined macros. Otherwise, there are 3 possibilities. - Undefined macros in preamble and %pre/%post/... scriptlets raise an error, unless a non-build parse is performed; this is to prevent malformed packages from being built. - In other cases, a warning is issued. - Some tokens are permitted beyond their context (e.g. %ghost in %changelog), to comply with traditional/sloppy usage. Also, undefined macros found in comments normally do not produce a warning. The idea is certainly not new, it was implemented in ALT Linux in October of 2005, actually. This feature proved to be useful in catching subtle specfile bugs caused by undefined macros. Said that, one should understand that a change that results to some previously acceptable specfiles being rejected is a policy enforcement. Whether it is a good idea to impose this specfile check without some grace period during which newly recognized specfile errors are reported but not yet raise errors depends on your user base, percentage of specfiles affected by the change, how long it would take to fix them all, etc. In ALT Linux, we chosen no grace period option and introduced %_allow_undefined_macros macro allowing users to relax the check. Of course, your mileage may vary. Finally, I'd like to thank Alexey for bringing this feature to a wider audience. -- ldv pgpeYIis_84RO.pgp Description: PGP signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [PATCH] Check for undefined macros
This change introduces a generalized routine rpmExpandMacros() to expand
macros on behalf of user input, with improved diagnostic facilities.
In particular, one of its arguments is a callback function which is
called whenever an undefined macro is encountered in user input.
When specfile parser calls this routine, it supplies a custom function
to handle undefined macros. This function is keenly aware of specfile
syntax: valid specfile tokens, such as %prep and %setup, when found in
the right context, are not treated as undefined macros. Otherwise,
there are 3 possibilities.
- Undefined macros in preamble and %pre/%post/... scriptlets raise
an error, unless a non-build parse is performed; this is to prevent
malformed packages from being built.
- In other cases, a warning is issued.
- Some tokens are permitted beyond their context (e.g. %ghost in
%changelog), to comply with traditional/sloppy usage. Also, undefined
macros found in comments normally do not produce a warning.
---
build/files.c | 66 ++--
build/pack.c | 11 ++--
build/parseSpec.c | 154 +++---
build/rpmbuild_internal.h | 11
build/spec.c | 3 +
rpmio/macro.c | 67 +++-
rpmio/rpmmacro.h | 28 ++---
7 files changed, 287 insertions(+), 53 deletions(-)
diff --git a/build/files.c b/build/files.c
index 10d7fff..881176e 100644
--- a/build/files.c
+++ b/build/files.c
@@ -1657,6 +1657,62 @@ exit:
return rc;
}
+/* Check if str looks like attr */
+static int eqAttr(const char *s, const char *a)
+{
+size_t len = strlen(a);
+if (rstreqn(s, a, len)) {
+ int c = s[len];
+ if (!(risalnum(c) || c == '_'))
+ return 1;
+}
+return 0;
+}
+
+int isFileAttr(const char *s)
+{
+/* parseForVerify */
+if (eqAttr(s, %verify) ||
+ eqAttr(s, %defverify))
+ return 1;
+/* parseForAttr */
+if (eqAttr(s, %attr) ||
+ eqAttr(s, %defattr))
+ return 1;
+/* parseForDev */
+if (eqAttr(s, %dev))
+ return 1;
+/* parseForConfig */
+if (eqAttr(s, %config))
+ return 1;
+/* parseForLang */
+if (eqAttr(s, %lang))
+ return 1;
+/* parseForCaps */
+if (eqAttr(s, %caps))
+ return 1;
+/* parseForSimple */
+for (VFA_t *vfa = virtualAttrs; vfa-attribute != NULL; vfa++)
+ if (eqAttr(s, vfa-attribute))
+ return 1;
+return 0;
+}
+
+/* Handle undefined macros */
+static void undefined(const char *fileName, int lineNum,
+ const char *s, const char *f, const char *fe,
+ const char *exp, int level, void *arg)
+{
+if (s + 1 == f isFileAttr(s))
+ return;
+rpmlog(RPMLOG_WARNING,
+ _(%s:%d: Undefined macro %%%.*s\n),
+ fileName, lineNum, (int)(fe - f), f);
+(void) exp;
+(void) level;
+(void) arg;
+}
+
static rpmRC readFilesManifest(rpmSpec spec, Package pkg, const char *path)
{
char *fn, buf[BUFSIZ];
@@ -1676,13 +1732,15 @@ static rpmRC readFilesManifest(rpmSpec spec, Package
pkg, const char *path)
goto exit;
}
+int lineNum = 0;
while (fgets(buf, sizeof(buf), fd)) {
handleComments(buf);
- if (expandMacros(spec, spec-macros, buf, sizeof(buf))) {
- rpmlog(RPMLOG_ERR, _(line: %s\n), buf);
+ char *exp = rpmExpandMacros(spec-macros, buf,
+ basename(fn), ++lineNum, undefined, NULL);
+ if (exp == NULL)
goto exit;
- }
- argvAdd((pkg-fileList), buf);
+ argvAdd((pkg-fileList), exp);
+ free(exp);
}
if (ferror(fd))
diff --git a/build/pack.c b/build/pack.c
index 9679d87..a8d1862 100644
--- a/build/pack.c
+++ b/build/pack.c
@@ -22,6 +22,7 @@
#include build/rpmbuild_misc.h
#include debug.h
+#include libgen.h
typedef struct cpioSourceArchive_s {
rpm_loff_t cpioArchiveSize;
@@ -91,12 +92,14 @@ static rpmRC addFileToTag(rpmSpec spec, const char * file,
}
}
+int lineNum = 0;
while (fgets(buf, sizeof(buf), f)) {
- if (expandMacros(spec, spec-macros, buf, sizeof(buf))) {
- rpmlog(RPMLOG_ERR, _(%s: line: %s\n), fn, buf);
+ char *exp = rpmExpandMacros(spec-macros, buf,
+ basename(fn), ++lineNum, NULL, NULL);
+ if (exp == NULL)
goto exit;
- }
- appendStringBuf(sb, buf);
+ appendStringBuf(sb, exp);
+ free(exp);
}
headerPutString(h, tag, getStringBuf(sb));
rc = RPMRC_OK;
diff --git a/build/parseSpec.c b/build/parseSpec.c
index fcec5c5..3b12d41 100644
--- a/build/parseSpec.c
+++ b/build/parseSpec.c
@@ -16,6 +16,7 @@
#include build/rpmbuild_internal.h
#include build/rpmbuild_misc.h
#include debug.h
+#include libgen.h
#define SKIPSPACE(s) { while (*(s) risspace(*(s))) (s)++; }
#define SKIPNONSPACE(s) { while (*(s) !risspace(*(s))) (s)++; }
@@
