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

[Steve Beattie]

On Mon, May 09, 2011 at 06:29:15AM -0700, Kees Cook wrote:
> 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?

Yes, absolutely. I have no problem revisiting this rather implicit
decision. As you can tell from most of the build, it's never been
very rigorously applied.

> 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.

Yes, entirely agreed. To me, the ideal goal would be that the default
build rules only show you the important messages, and compiler warnings
and errors are most definitely those.

I didn't want to do the stderr redirection on the unit tests when I
originally added them, but there was enough cross-dependent symbols
that I was either linking in everything, and thus getting the real
main and needing to fake more and more state, that I went with the
weak lining option, which then generated the copious warning output,
and it was again the complaints from other developers about the
"noisy" unit test warnings that caused me to implement. But if the
unit tests no longer generate spurious build warnings, then that
redirection should go away.

> > 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.

Maybe I'm missing what you're proposing, but the 'check' target
currently exists. However, the unit tests themselves aren't a separate
target; if you do make check, you get the unit tests, the parsing
tests, and the caching tests. The latter two (IIRC) you can target
individually within the tst/ subdirectory, but not the unit tests.

> > (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.

Hrm, I find the "I'm changing into blah directory" output generally
not very useful, but then I'm probably much more familiar with a lot
of the details of the directory layout then others. I can perhaps poke
seeing if .SILENT can be turned off for verbose builds.

> 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).

Yes, again, it should be fully possible to get all output in order
to diagnose build failures, and for distribution vendors to see
full compilation options so that they can verify that any specific
requirements that the platform has are getting applied.

> > > -	rm -f libstdc++.a
> > 
> > Is there a reason you dropped this?
> 
> Yeah, it's covered by the *.a in the first line of the "clean" target.

Oh, hah, yeah. I'm not sure we generate any other '.a's that would
need to be cleaned up.

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

Thanks for pushing on it!

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20110510/93535a8c/attachment.pgp>