[apparmor] [PATCH] parser_common.c and unit test cleanups

[Kees Cook]

On Sat, May 07, 2011 at 07:28:10PM -0700, Steve Beattie wrote:
> > and remove all the build-log silencing (since reviewing
> > a failed build log wasn't useful without VERBOSE=1).
> 
> I'd like to keep build silencing as much as possible so that in a
> standard build only important things are shown, like compiler warnings
> and test failures, and then keep the VERBOSE or V = 1 option available
> for diagnosing specific bits of the build process. We've had issues in
> the past where because the build output was deemed "too noisy" by some
> developers, running through the tests would be skipped during the
> development process.

Okay, sure. I can change that around. I saw that the non-test code was
built without Q, so I guess I went in the wrong direction with it. It
sounds like it would be preferred to do everything with Q?

Can we perhaps discuss a general "policy" for our build out?

I would like to not hide warnings and errors, though, so I'd like to vote
for not redirecting stderr. We've worked hard to actually fix all the
warnings, so they should stand out when we hit them in the future.

> The unit tests should probably gain their own make target to ease
> running them separate from the other tests, given the length of the
> language parsing tests.

I've actually done just this in an upcoming patch. I actually split out
three targets: "indep" "arch" and "check". This was to avoid building the
parser on Hurd, etc.

> (I admit I'm a bit schizophrenic on this point, as e.g. I'm the one that
> added the option to flex to dump scanner statistics to stdout. I should
> probably submit a patch to only add that when VERBOSE is set.)

Right. Let's settle on a common pattern. Default would be to use Q
(@-prefix) for all build targets (but not redirect stderr). But since
VERBOSE doesn't control .SILENT targets, I'd like to drop the use of
.SILENT.

I feel that builds should report what it is working on, but I can
understand the desire to suppress the actual recipes being executed.

As a packager, I would build everything with VERBOSE because I want to be
able to see the specific actions that are being called on a given build
(since many of the builds may be happening on hardware I don't have
immediate access to).

> > -SRCS = parser_include.c parser_interface.c parser_lex.c parser_main.c \
> > +SRCS = parser_common.c \
> > +       parser_include.c parser_interface.c parser_lex.c parser_main.c \
> 
> I know you're trying to minimize the diff to ease review, but go ahead
> and re-adjust these lines.

Heh, okay.

> > -	rm -f libstdc++.a
> 
> Is there a reason you dropped this? At compilation, we create the

Yeah, it's covered by the *.a in the first line of the "clean" target.

> libstdc++.a symlink so that we can statically link the parser against
> libstdc++ to keep the parser's dynamic links against things that should
> be in /lib/, to make the parser usable for early booting situations
> (libstdc++'s dynamic library lives in /usr/lib, making it unsuitable
> for situations where /usr is nfs mounted).

Right, I retained all of that logic; it is very important. It actually is a
.PHONY target to make absolutely sure it is always getting the current
libstdc++.

I'll send updated patches; thanks for looking it over!

-Kees

-- 
Kees Cook
Ubuntu Security Team