On 08/16/10 03:59 AM, Tim Foster wrote:
Hi Brock,
Thanks for the really thorough review - I really do appreciate the time
it takes to go through a 4,600 line webrev and send me feedback.
Comments below,
On Fri, 2010-08-13 at 20:53 -0700, Brock Pytlik wrote:
On 08/12/10 07:45 PM, Tim Foster wrote:
I've got another pkglint webrev here that I'd really appreciate code
review comments on please?
http://cr.opensolaris.org/~timf/pkglint-webrev-2
[snip]
248, 252: signature actions should be allowed as part of renamed and
obsolete packages. Since this may not work outside of pkglint after my
putback, I'm fine with this being an rfe.
I've logged 16800 for this.
Thanks
[snip]
79+: our other man pages don't look like this but I've heard we're maybe
moving in this direction? Either way, probably worth getting clarification.
Yep, I think 'nroff -man' wasn't quite enough. With
$ tbl pkglint\-man\-src.txt | \
neqn /usr/share/lib/pub/eqnchar - | \
nroff -u0 -Tlp -man - | col -x
(basically, what man(1) does) it looks better - I've put that text file
into the updated webrev for now.
Great, thanks.
lint/base.py:
I couldn't quite follow the relationship between pkglint_id and name.
name is the overall short name for a given Checker module, used in lint
messages. pkglint_id identifies a given checker method inside that
module. Combining the two gives us something short enough to appear in
lint messages and uniquely identify the source of that message. 'pkglint
-L' output shows how these correspond back to the sources.
Ok, that makes sense, thanks.
I think some explanation of what an engine is, and a general idea of how
the pieces fit together here would be useful. For example, how do I know
if I'm defining a new type of check or creating a new instance of an
existing type?
Sure. LintEngine is essentially a Facade to the rest of the pkglint
backend. In normal usage, you'd probably only ever want one of these,
though nothing would prevent an API user from creating more (say, to
perform two lint runs in parallel on different sources, and compare the
results)
Checker subclasses implement the checks themselves, with each object
building a list of those implementing methods. The engine instantiates
Checker objects, and iterates over their list of checker methods,
calling each for every manifest and action presented for linting.
I'm expecting all pkglint plugins to be subclassing ManifestChecker,
ActionChecker or (eventually) ContentChecker, implementing their own
checker methods.
I think I'm following this. Since a checker class can have multiple
checker methods, when is it appropriate to add a checker method to an
existing class versus creating a new checker class for the method?
Why is the base Checker doing a check for conflicting variants?
That method is just there as an implementation for the subclasses to use
- Checker itself doesn't do any work other than providing a common
__init__ method.
That makes sense
[snip]
util/publish/pkglint.py:
I would have expected this to be in src/, or at least in the same
location as pkgsend/recv/sign etc... Do we have clear guidance on when
things belong in util/* and when they don't?
I don't mind moving it - it just felt like it belonged closer to
pkgfmt(1) than the other tools, being something we would only use during
the authoring process. I've left it where it is for now.
I can see that making sense too.
lint/engine:
[snip]
79-85: I don't remember seeing explanations of what a reference or lint
image are by this point.
Ok, I think this is a bit better:
User-supplied manifests for linting are read directly as files
provided on the command line. For cross-checking against a
reference repository, or linting the content of a repository, we
create a reference or lint user-images in a provided cache
location, used to obtain manifests from those repositories.
That works for me.
104-107: This doesn't really seem to be a sentence, and I'm having
trouble unpacking the clauses.
If I understood correctly, here's a suggested rewrite:
Determines whether to perform a subset of checks which may only make
sense for published packages on manifests passed from the command line.
The option also applies to manifests from a repository, so
Whether to perform checks on manifests which may only make sense
for published packages.
is probably better.
Ok.
214: could "pkglintext" have some separators added to it? "pkg_lint_ext"
"pkg.lint.ext"? If it's too late for that change, no problem.
Absolutely. I'd prefer pkglint.ext though, if that's ok (since I already
have pkglint.exclude)
pkglint.ext works for me.
255-280 (and other places in other files too I think): This is just a
personal pet peeve, but could we choose another format for this? If we
ever added another argument to the function with a name longer than
"lint_manifests" then we have to go back and reindent all of this text,
which is both a silly annoyance and also clutters up the diff.
Sure, I've changed that here and in other docstrings.
Thanks :)
[snip]
401: Is there a reason to make passing None as the value of api_inst
doesn't cause an error instead of returning immediately?
It simplifies other areas of the code - rather than checking for a valid
api_inst every time we're looking for reference or lint manifests, if we
haven't configured an api_instance, we just drive on as if they never
existed.
This happens normally, for example when we're linting manifests provided
on the command line and aren't linting against a reference repository.
During LintEngine.setup() we'll already have thrown an exception if
api_inst is unexpectedly set to None.
Ah, ok. I wasn't clear why None made sense as a value there. I get it now.
445: Should this be> instead of<? Looking into log.py, I guess not.
Intuitively, I would have expected more debugging to be represented by
bigger numbers, but it's not a huge deal to me either way.
Python's logging framework as well as java.util.logging have similar
directions of logging levels, so thought it best to stay consistent.
Yeah, I saw that after I sent my email. I agree it makes sense to follow
their convention.
So we only show the tracker in DEBUG level of verbosity? I think I
would have expected the tracker to be shown by default.
I did experiment with that, but found that when not redirecting stderr
to siphon off lint messages, the output looked pretty messy as the lint
errors would co-mingle with the progress tracker. I could try to fix
that if you think the tracker would be better - certainly on very large
lint runs (3,000+ packages) that spend a long time in engine.setup(..)
the tracker is nice to have.
Hmm, I had missed that the tracker would interleave w/ the lint
messages. Perhaps a switch to turn the tracker on then? I'm working from
memory now but I thought that going to debug level increased the amount
of output in general. It might be nice for a user to be able to turn the
tracker on (or off) independently from the logging level?
I don't have strong feelings about this though so if what I'm saying
doesn't make sense or is a pain to implement, don't worry about it.
[snip]
I'm confused by the comment on lines 533-534 about only supporting one
publisher per image, and the code on lines 561-586 which seems to be
adding multiple publishers to the image.
Yes, it was a last-minute fix to restrict the code to a single
publisher: I should have been more thorough in removing the older
implementation until I get a chance to work on multiple publisher
support. Fixed.
Gotcha
[snip]
Also, is the check on line 633 redundent with the one on 613 or do you
anticipate being able to check content without checking an action in the
future?
Good catch, it's redundant for now (but see below). At some point in
the future, we could perhaps come up with different values for
'pkg.linted' allowing us to only bypass particular classes of check, or
even particular checks - without a specific use case, I think that's
overkill at the moment.
Ah, I see now. That makes sense and I'm fine w/ leaving things as you
had them.
[snip]
lint/opensolaris.py:
51: Probably just my own lack of knowledge here but I thought we were ok
with usernames longer than 8 chars.
That sounds familiar, but passwd(4) still says 8 characters and
useradd(1) enforces an 8 char maximum by default on my machine.
Ok
[snip]
99: why not just base.ManifestChecker.__init__(self, config)? (same
comment for the other checker modules)
I'm actually not sure, both are valid. I'll dig into which is the
preferred way to do this, and why.
Don't worry about this too much, it just surprised me because it looked
different than other init's I was familiar with. Either is ok with me.
[snip]
lint/pkglint_action:
152-153: I don't understand this comment, mostly the notion of search
pattern here has me lost.
Perhaps 'search' was the wrong word - it's just a regexp to match narrow
the list of packages we're interested in linting.
[snip]
203: Was the check with id 002 removed?
Nope, looks like there was an error in an old version of the code where
I had:
---
def dup_paths(self, action, manifest, engine, pkglint_id="001"):
"""Checks for duplicate paths on non-ref-counted actions."""
self.dup_attr_check(["file", "license"], "path",
self.ref_paths,
self.processed_paths, action, manifest, engine, id="002")
---
The original veresion of dup_attr_check only did file paths, before I
made it generic to do multiple attribute-types: I got the numbering
wrong while making that change.
I've re-numbered the checks in that class so that they're contiguous
(better to get this right now - it would be wrong to re-number after it
gets putback)
Are the id's eventually expected to be an interface with some reasonable
level of stability? I hadn't realized that the ids were more or less
assigned for all time.
[snip]
538: I'm having trouble understanding what this is trying to check. Is
"us" pkg5?
Nope, 'us' is the package containing the legacy action. This is
checking that packages declared by the 'pkg' attributes of legacy
actions should depend on the package containing that legacy action.
I've reworded the docstring.
[snip]
lint/pkglint_manifest:
[snip]
286-287: I don't understand why @ .0 having a timestamp means it's newer
than one without a timestamp while @ .1. it means it's older than one
without a timestamp.
This is all about determining when to overwrite dictionary entries
generated from a reference repository with what's being presented for
linting. (and thus, we presume, potentially published to that reference
repository)
In particular, we're ignoring the timestamp field if present, since
that'll get updated once we publish the package - so we're biasing the
comparison to treat packages from the lint repository as being newer if
the only thing different between a lint package and a reference package
is the timestamp.
Hmm. Ok, I get the idea of preferring packages from the lint repo over
packages from the reference repo. What I'm having trouble connecting is
why a different decision is made with regard to which is newer depending
on whether both packages have a version @.1 or @.0. What about .2
through .9? Is it the second position that matters or the last one?
It's possible that I'm just not groking the example, but how I interpret
it is "When the package version ends in .0, we use one set of rules
where having a time stamp means you're newer than not having a time
stamp. When the package version ends in .1, we use a different set of
rules where having a timestamp means you're older than not having a time
stamp." Also, in your example, I'm not seeing any mention of lint versus
reference repo, so maybe that's the step I'm missing?
util/pkglintrc:
1: I'm not sure what that tag means or when I would (or would not) put
it in my own pkglintrc file.
It's a default section header for the config file - without it,
ConfigParser chokes. I've used a better name for the pkglint section,
"pkglint" and updated calls to config.get(..) and friends.
Ah, gotcha.
util/publish/pkglint:
having an option to direct the output to a file would be nice
Lint output, or all output? All lint output goes to stderr, everything
else goes to stdout.
Ok, that works.
I'm a little concerned about how the logging interface is being used
here. Mostly, it seems like if someone ever wanted to build an alternate
interface on top of this code, it would mean having to parse the strings
in the logger output. I realize it's probably late in the game to switch
that out, but would you consider filing a low priority RFE about it?
Ok, which bit of the logging interface? It was written precisely so it
could be extended in the manner you suggest - perhaps I'm not
understanding your point?
My concern is that we're logging messages, rather than collecting
objects and later converting the objects to strings. As an analogy, this
has a similar feel to raising strings as exceptions instead of having a
class hierarchy for exceptions.
For example, if I wanted to make graphical interface which colored
packages according to the types of lint errors they contained (Content
vs Action vs Manifest), I don't think there'd a way to do it short of
parsing the error messages and maintaining a mapping from id -> type
some place.
I don't see us doing that anytime soon, which is why I mentioned a low
priority RFE.
A good example of using the lint API, overriding the default log
mechanism is in src/tests/api/lint/t_pkglint.py where we get the
lint_ids and the text of the log message, and buffer them during a lint
run to examine later, separating the message text and the id.
I've added some comments to the snippets below
----------------------------------
import pkg.lint.log as log
class TestLogFormatter(log.LogFormatter):
"""Records log messages to a buffer"""
def __init__(self):
self.messages = []
self.ids = []
super(TestLogFormatter, self).__init__()
def format(self, msg):
if isinstance(msg, log.LintMessage):
if msg.level>= self.level:
self.messages.append("%s\t%s" %
(msg.msgid, str(msg)))
self.ids.append(msg.msgid)
def close(self):
self.messages = []
self.ids = []
----------------------------------
then further on,
----------------------------------
lint_logger = TestLogFormatter()
lint_engine = engine.LintEngine(lint_logger)
# read an array of local filenames, returning an array
# of the contents of those filenames
manifests = read_manifests([manifest], lint_logger)
lint_engine.setup(lint_manifests=manifests)
lint_engine.execute()
lint_engine.teardown()
# now examine lint_logger.ids, etc.
----------------------------------
The default LogFormatter emits log messages to its own private python
logger, writing to stderr, but there's no reason why API users couldn't
initalise the engine with their own LogFormatter object similar to the
above.
[snip
66-83: Could you comment on this approach vs the one we've used
elsewhere? Would you suggest we should look to move all our code to this
approach rather than our current one?
It's force of habit mostly - it's what I used before when doing command
line option parsing and I feel it's a lot easier to use than getopt.
ISTR Rich Lowe running into an i18n bug with getopt that wasn't present
in optparse too. That said, optparse has apparently been deprecated in
2.7 (which I didn't know before digging around for an answer for you) to
be replaced by argparse There's a full discussion at
http://www.python.org/dev/peps/pep-0389/#why-aren-t-getopt-and-optparse-enough
Now that I read that, I really look forward to the subcommand support of
argparse. Even without argparse though, I'm still eyeing up fixing
12723...
I guess I'm willing to switch to getopt if people want me to in the
meantime for the sake of consistency?
I don't have strong feelings about it, though it being deprecated makes
me a bit concerned. Perhaps Danek or someone else will have a stronger
opinion in this area?
And argparse does look like it'll be an improvement over what we have.
228: Why codecs.open? Is this something we should be doing in other
places across the code?
It tries to read the file as UTF-8, and will complain if non-UTF-8
encoded characters are present. I think we should be using it in places
where we don't control the encoding of files being passed to us in order
to fail immediately rather than silently passing the broken content
upwards, or dropping it. Most of the publishing tools I think.
Ok, that makes sense.
Thanks,
Brock
This looks like a good approach, I'm looking forward to having it in the
gate and getting our builds lint clean. Thanks,
Thanks again for the review - I've posted an updated webrev, and an
incremental webrev at:
http://cr.opensolaris.org/~timf/pkglint-webrev-3
http://cr.opensolaris.org/~timf/pkglint-incr-webrev-3-vs-2
In summary, the changes discussed in this mail, and included in the
webrev are:
remove pkglint-plan.txt
reformat docstrings using the same style as other modules
clean up comments, optimise conflicting_variants (comparison is commutative)
rename pkglintext to pkglint.ext, reformat manpage
docstrings shouldn't use columns for argument descriptions
remove ContentChecker framework (no implementation yet)
check for action/manifest 'pkg.linted' value.lower() == "true"
remove redundant check for invalid chars in username
remove XXX comment about missing functionality, filed rfe instead
remove opensolaris.zone check for ref counted actions
re-word legacy action check docstring
remove superfluous 'manifest' argument to calls to dup_attr_check(..)
re-number pkglint_ids for dup action checks
Fix indentation in usage message and in pkglint_manifest.py
Rename [DEFAULT] section to [pkglint] in pkglintrc
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
