Code reorg - #234
Conversation
b2b7a57 to
61783ff
Compare
| # XXX instead of magic handling of tarfiles, it'd be nicer | ||
| # to have separate keywords for shipping archives. | ||
| # Since it only seems to make sense for pre-built plugins | ||
| # and their dependencies, perhaps plugin_archive. |
There was a problem hiding this comment.
I suspect this could be implemented on top of #236, maybe let's add a comment there and I can work on incorporating it, or let's add an issue sketching what you had in mind.
|
Okay Benjamin, I'm getting on this one now — will work in edits and rebase, then take out of draft. |
61783ff to
6c8d8a4
Compare
This moves all directory information into a new Config class that captures zkg's internal configuration, based on its config file. The motivation here is that the Manager class does not need to manage this information, and by making it available from a central configuration, other classes can also leverage it (as opposed to having to live in the Manager). It includes various consistency fixes: the zeek_dist path is only used when actually present, and saving the configuration always uses a canonical ordering (alphabetically, first by sections, then by options in each section).
The tests shouldn't reach out to Github for the default package source, unless they explicitly need to.
Given that we already have a class for package sources, it makes more sense for that functionality to live in that class, instead of the manager. This also introduces a small tweak to the behavior of the refresh command, prohibiting the use of "--push" without "--aggregate". There is no other local modification to source repo clones.
6c8d8a4 to
a1a3a45
Compare
|
Alright! This is a full rebase and things seem to be working pretty well. This still has minor feature changes, which I think we should actually aim to keep for Zeek 9 (particularly aligning the handling of test and build logs, putting them in one place). But we can take that out of this one for subsequent PRs, doesn't matter to me. |
|
Btw I don't feel strongly about whether this should make Zeek 9 still. It's a big change to mainly put in place cleanups. I could also see shipping what's currently in master with 9 (so your new bundling approach etc), and considering this PR the starting point for all of the upcoming work. |
2b76cf6 to
9eae9d5
Compare
Also, no longer create the `zkg.test_command` logs buried in the test clone. This mirrors how we already produce the `<package>-build.log` in the log directory for packages that have a `build_command`.
This avoids circular dependencies when removing the amount of from-import statements into the toplevel zeekpkg module, in the next changeset.
The zeekpkg.cli module produces the argument parser and contains the various commands. It leaves a single print() in zkg.py (redundantly to _util.print_error()), which is temporary until we introduce a UI abstraction.
This further shrinks the toplevel zkg script.
The "zkg list" command would previously preserve any opening quotation marks, but since it cuts off the description after the first sentence this would look broken. Such surrounding quotes are now never rendered in the output.
This provides UserInterface, an abstract base class for user input/output. Implementations can adapt to the environment (TTY, file I/O, etc). It includes one implementation, PlainUI, for basic plaintext I/O. Future additions might be textualize/rich etc. This also generalizes the notion of InstallWorker into a range of activities undertaken by zkg, which allows treating trackable ones via a progress bar, etc. This does not alter the current split of messages to stdout/stderr. It tweaks the message prefix slightly for consistency, which touches a few baselines but makes no deeper changes otherwise.
There is no more templating around PY_MOD_INSTALL_DIR etc.
9eae9d5 to
c6fdb8a
Compare
|
All yours Benjamin. I missed one change in our conversation today, 40f995d. It'd be nice to have because it's long been too onerous to locate test log output, but we can take it out of this PR. If I've not addressed any of your comments at this point, that's an oversight. |
There was a problem hiding this comment.
Thanks for iterating. I went through all the issues and resolved most of them. I left a small number open which were about creating concrete follow-up issues.
After that I made another pass and ran a few tests, and found a few remaining regressions which I opened issues for. I believe most of these should be simple to fix.
bbannier
left a comment
bbannier
left a comment
There was a problem hiding this comment.
Thanks, looks good for me but for a tiny regression, opened a new issue.
I went through and again resolved all addressed threads. We still have open issues where you wanted to create issues for existing bugs/shortcomings, could you do that before moving on?
| zeekpkg.UI.error("Permission problems prevent zkg from creating internal state") | ||
| if not args.user: | ||
| zeekpkg.UI.error( | ||
| "Consider the --user flag to manage zkg state via {home_config_dir()}/config", |
There was a problem hiding this comment.
This was an f-string before.
| "Consider the --user flag to manage zkg state via {home_config_dir()}/config", | |
| f"Consider the --user flag to manage zkg state via {home_config_dir()}/config", |
2 participants
@bbannier this is the bulk of the changes I've been sitting on that we've discussed. In essence it's moving all of the commands out of zkg into zeekpkg.cli, the introduction of a config class, a UI abstraction, and some minor tweaks.
Some minor behavior changes slipped in there that don't need to be there, such as 04a8580. We could postpone these until later — I don't feel strongly.
I'll make this a draft PR since I imagine you have opinions about it, and I don't mean for this to cause conflicts with other PRs in flight.