Conversation
747d7cd to
38d998e
Compare
timwoj
left a comment
timwoj
left a comment
There was a problem hiding this comment.
I honestly made it all the way through with zero comments. I read through the design document and the changes here appear to cover everything you wanted.
There was a problem hiding this comment.
I've made a first real pass over this. To be clear Benjamin, I am treating this as agent code, not yours. The commits are definitely nice and manageable — very cool.
Now ... I agree that this PR gets the job done! It's impressive. But it also feels like it's it's just bolting in the functionality via ifs and additional functions, and I can't say I like the resulting code better in the end. Low-level git code remains all over the manager ... perhaps it has to, but my hunch is we can do better. The one comment that says a more structured design is TODO is really the essence of this.
I think we could use this as a release candidate, or more likely a feature preview, that we can take to people to see how they like it. I don't think this should go into Zeek 9 before we've spent more time thinking through the design. I'd next take a fresh look at our class layout, figure out how to represent packages with different storage implementations, and then work that in.
38d998e to
aa9e703
Compare
There was a problem hiding this comment.
Okay Benjamin, I've now spent a lot more time on this and the good news is that it seems to be working as intended in my local testing. I like that it seems pretty narrow — I hadn't realized you need a local package tree that is also not a Git repo, plus presence of a version key, for the new logic to trigger. I also like the explicit disabling of such packages for bundles, which I think is pretty important at this stage.
One thing — I wonder if this is actually too narrow. If the directory approach never kicks in if a local tree is a git repo, that still doesn't help much during the typical development workflow, where you do work in git — you just don't want your package install to be gated on git complaints.
So I'm wondering whether we should make the tracking explicit. We could have zkg install --directory, perhaps, or support it in zkg.meta. If we add something like
[package]
tracking = git
as the default, and
[package]
tracking = directory
as the override, then you could work with git and have zkg ignore it. You can toggle as you see fit during development, which is cool too. I'm fairly sure this is nicer than relying on presence/absence of a version tag, which is pretty implicit. The version could also simply default to something like devel when not given while tracking is directory-based.
A final, minor thing: I'm not seeing much (anything?) regarding directory tracking in the output of zkg -vvv. It would help to make that pretty clear so there's no uncertainty about how zkg handles the package.
A few other comments in line, but I'm mostly coming around on this one. 👍
aa9e703 to
2e906e1
Compare
|
Thanks for the review. Somehow I lost the actual non-Git functionality while doing a rebase, and now put it back. I still need to work through your other feedback so this is not getting ready today. |
I fixed one remaining issue, but I think this should work now since when working with a package we check whether the passed package name is a valid local path and if it does use the directory backend. The intention is to seamlessly enable the directory backend so that uses like
Even though this was broken in some intermediary version, my intention was always to make this implicit. My motivation was that users working with local checkouts are typically package developers working on their own packages who want All this said, I'd prefer to keep this implicit. WDYT?
I added some log output when the directory backend is being used. |
We consolidate version-tag lookup, branch detection, commit-hash recognition, checkout, and outdated-check into one function that returns a `GitResolution` dataclass. Call sites in `test()` and `_install()` are updated to use it. This prepares for upcoming non-Git package sources by making the resolution boundary explicit.
Unit tests are cheap and catch logic errors quickly; BTest integration tests are slow. Running them in this order surfaces most failures without waiting for the full end-to-end suite.
`_pick_version` selects the version string and tracking method from the clone's available refs; `_resolve_git_version` reads the resulting HEAD state after checkout. Callers are now responsible for checking out the desired ref before calling `_resolve_git_version`. This prepares for upcoming directory-backed package support, where version selection happens outside the snapshot abstraction.
Decouple package-info gathering from Git operations so Git and directory sources can be handled uniformly by downstream install, test, and info paths.
The function now takes a `PackageSnapshot` rather than a `git.Repo`; the new name reflects that.
Add `file://` URL handling in the CLI so users can explicitly force the Git backend for a local path. `check_local_git_repo` strips the prefix before validating the working tree, and `active_git_branch` strips it before opening the repo.
Extend install, test, info, refresh, and upgrade to support directory-backed packages alongside Git-backed ones, using the snapshot abstraction from the previous commit.
Catch a mismatch between the `version` field in `zkg.meta` and the Git tag being installed before the install completes. Provide an opt-out for cases where the mismatch is expected.
… path A directory package passed to `bundle()` with `prefer_existing_clones=True` would crash trying to open its path as a `git.Repo`. Guard against this by rejecting directory packages early and skipping the existing-clone fast path for non-Git installed packages.
Expose the `skip_version_validation` parameter added to `Manager.install()` via a new `--skip-version-validation` flag on `zkg install`, so users can opt out of version-field validation on the command line.
\`zkg install\`, \`zkg test\`, and \`zkg bundle\` now accept plain local directories in addition to Git repos. A local path starting with \`.\` or \`/\` that is not a Git repo is routed to the directory backend introduced in the previous commit. For directory-backed packages, \`best_version()\` returns nothing since there is no Git history. Fall back to \`metadata_version\` instead, which reads the \`version\` field from \`zkg.meta\`.
The function now accepts plain directories in addition to Git repos; the new name reflects the broader scope.
The value always equals `snapshot.tracking_method`, which is set by both `_snapshot_from_git_repo` and `_snapshot_from_directory`. Read it from the snapshot directly instead of threading it through every call site.
The method requires a Git clone to enumerate tags. All callers already check `tracking_method` beforehand, but the function itself had no guard. Add an assert and document the contract in the docstring.
…ion` Previously the warning only stated the discrepancy. Now it also tells the user the install proceeds despite the mismatch.
The trailing `return True` was unreachable. Rewrite the docstring to explain the three accepted cases (remote URL, file:// with clean tree, local directory) rather than describing implementation steps.
Replace vague "source-specific resolution" wording with concrete description of what the class represents and when it is constructed.
Users running with `-vvv` can now see when directory tracking activates and which version was resolved from `zkg.meta`.
The `version` field in `zkg.meta` is no longer required for directory-backed packages. When omitted, `snapshot.version` is `None` and version-spec checks against such packages are rejected gracefully.
A mismatch between the zkg.meta `version` field and the Git tag is always an error now. This removes the CLI flag, its plumbing through `install()` and `_install()`, and the associated btest.
cd1fe04 to
179fa96
Compare
3 participants
Caution
The most offensive bit upfront: This PR was fully generated with Claude Sonnet 4.6 (all code, tests, even commit messages). This PR description is written by me. I started this out with a design doc written by me which captures some points from #231 (full of grammar errors and typos, human!). I added lots of instructions to make sure the LLM-generated commits are up to our standards and are fit for human review, and to keep it from falling back to its default, unacceptable coding style. I also requested good unit test coverage for new code, something which we currently do not have. If not automatically generated, I requested BTest integration tests as well.
Part of my instructions was to create self-contained commits which are easy to review. I did a review of all changes in this branch, so I can be and am fully accountable for anything happening here.
What this accomplishes is that we now support running operations like
zkg installorzkg testagainst plain directories. This lifts the requirement to always commit stuff to Git before e.g., being able to run tests, and on the side also closes #158. For directories we do not clone into the local staging area, but instead work directly from it. This means that if e.g., a package requires a build step this can be shared across multiple invocations.Since we do not require network access for such "directory packages" local directories also improve on #176 (I did not make this explicit with a flag though). The design here should also work for in the future adding support to install from tarballs which would allow users to e.g., bundle up pre-compiled artifacts without having to commit them to Git; I could imagine tarball support to extract to a tempdir, and install from that.
Since directory packages have no Git tag to determine its version, we require an explicit
versionentry inzkg.metafor them. Down the line this will help implement #211 since we can decouple per-package versions from Git tags on the monorepo.