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 > General comment: > Indentation on docstrings seemed inconsistent w/ the other files we have > in most of your new files. Yes, you're absolutely right, sorry. There's a few other files outside this webrev that have inconsistent docstrings too (client.progress and cfgfiles) I've fixed my ones. > doc/pkglint-plan.txt: > nit line 100: can you accept multiple repositiories or only one? ("a > repositories" is what's there now) I'd actually meant to remove pkglint-plan.txt from the webrev - I'm not sure the documentation there is useful (though it is current) I'd originally intended 'repositories' but the current implementation only supports a single lint & reference repository. > 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. > man/pkglint.1.txt: > lines 11-13: This don't seem to be following the indenting conventions > of our other man pages. I had converted the man page for pkglint to troff format and had been using that privately, having gotten annoyed at trying to duplicate exact man-page formatting using a plain text editor. I submitted a .txt version of that for this webrev. I saw the recent post to pkg-discuss asking about the format of our man pages, so I'm willing to putback that troff source if that's the direction we eventually want to go, rather than the copy/pasted output of 'nroff -man' on my local source file. Webrev has an easier time dealing with plaintext, I admit. > 42-43: what happens if I combine cache and -l and -r? If lint and reference images don't exist in the cache dir, they'll be created, with the publisher set to either the lint or reference repositories. If images already exist, we emit a warning saying that the -l or -r option is being ignored, and we load the cache instead. Once pkglint supports multiple repositories, if the cache dir contains images that don't already have publishers set to the -l or -r options, they will be added as publishers to the existing lint or reference images respectively. > 50: Is "/* LINTED */" a convention known outside ON/Oracle? This > paragraph would have made more sense to me if the first sentence was > something like: > pkglint(1) allows package authors to bypass checks for a given manifest > or action. Sounds fair enough - Google doesn't yield many hits for 'LINTED' - I found http://plan9.bell-labs.com/7thEdMan/vol2/lint, which does mention many of the keyword-comments to tune traditional lint behaviour, but LINTED isn't one of them. > 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. > 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. > 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. > 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. > It's probably worth including a note that if a new type of check is > added here, the code in lint/engine.py will need to be changed so that > the new type can be used. Yes. > 40-59, 131-132, 142-144, 103-104: indentation looks wrong Thanks - docstrings fixed as per the general comments above. > 64: Can there only ever be one instance of a Checker object in existence > at a time? I'm not seeing why the ConfigParser object should be shared > among them. You're right, thanks - there can be many Checker objects, the config object should be per-instance, not per-class. I've added a docstring to Checker to point to pkg.lint.engine where we document the keys we expect to find in it. > 93: what's the type of item? I'm mixed up here alright. check() here is redundant, given that we never instantiate Checker objects, always subclasses where the check() signature differs, with well-defined arguments. I've removed it and added text to the docstring to explain. > 109, 115: It seems like if 109 enumerated the items as they were > iterated over, then 115 could start at i+1 instead of looping over the > entire list since, as you mention, intersection is commutative. Thanks, that's much nicer, fixed. > I'm also > picking up my separation of the VariantSets class into two separate > classes with different functions. I'd like to talk with you offline > about what I'm looking at doing to make sure that what you need will > still be provided with the changes I have in mind. Cool, that'd be great, thanks. > 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. > lint/engine: > 51, 61-138, 182-184, 196-197, ...: indentation looks off Yep, fixed the docstrings as above. > 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. > 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. > 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) > 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. > 354: extra line Thanks. > 377: shouldn't there be an error/exception/warning raised if there's a > checker which isn't a subclass of the 3 expected classes? Yes, added that. > 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. > 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. > 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. > 477-479: weird capitalization here Thanks > 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. > 593, 613, 633: So having pkg.linted=False or pkg.linted=True would both > cause the action/manifest/content to not be checked? That's true (and incorrect on my part) I've fixed it here and elsewhere to look for a case-insensitive "true". > 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. > 625: I don't quite follow the XXX comment. My mistake. Content checking isn't implemented yet - I should really have removed that comment, the method it's attached to, and the ContentChecker stub: it's just confusing. Rather than checking actions or manifests, ContentChecker was supposed to be a way to check the individual files that a package delivers. In order to do content checking, more work is needed for pkglint(1) to accept -d arguments, and have the engine search those directories for files to be linted (for local manifests that point to those files), and add code to pull the payload from lint or reference repositories, finally passing references to that payload to a ContentChecker object which will perform checks on those files. There's no point in having half the framework there with nothing to use it - I've removed it. > 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. > 66: I think the ":" check is redundant given the re that follows (ie, no > username with : in it could match that re). Yes, thanks. > 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. > 106: why is this XXX'd? It never got implemented - I've removed the XXX and filed 16802 for it. > 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. > 181: A brief comment here about why we want to merge the lint and ref > dictionaries now but it was ok to have the lint_manifests and the > manifests from engine.gen_manifests(engine.linst_api_inst ... ) put > into the same dictionary from the start would be helpful. Ok. > 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) > 273: The XXX should either be removed from the comment or the code on > line 279 should be removed? Yes, I've removed the check for opensolaris.zone - it looks like that attribute has been eradicated in the current version of the dev repository. The docs/tags-and-attributes.txt file notes these as obsolete, being replaced by variant.opensolaris.zone. > 310, 374: The manifest parameter doesn't seem to be used? You're right, I've removed that. > 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. > 540: missing comma or period between us and check? Yes, thanks. > lint/pkglint_manifest: > 234-237: wrong indentation egads, how did that one get through - sorry! > 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. > > 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. > 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. > 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? 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. > 60-63: same comment about indentation as in the man page/ Fixed. > 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? > 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. > 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
