Implement support for packages not backed by Git repos - #236
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. 👍
| class PackageSnapshot: | ||
| """A fixed, immutable view of a package at the point it enters processing. | ||
|
|
||
| All source-specific resolution must be completed before constructing this |
There was a problem hiding this comment.
A detail: I'm not sure what "source" and "resolution" mean here. I think it means the package needs to exist on disk and its current state must match that spelled out in the arguments to the constructor, right? (But that conflicts a bit with None for tracking_method being "not yet determined".
There was a problem hiding this comment.
I updated the class documentation, PTAL.
| manager: zeekpkg.Manager, | ||
| package_name: str, | ||
| package_version: str, | ||
| skip_version_validation: bool = False, |
There was a problem hiding this comment.
When I got to this commit again (on my second pass), it still wasn't clear to me under which circumstances you'd want this. Wouldn't it be better to just call out that there's a discrepancy, and it's not clear what to do?
There was a problem hiding this comment.
Not absolutely sure either, it felt right to allow users to opt out given that is a new consistency check. A scenario where this could become interesting would be when a single commit has multiple, conflicting Git tags; in that case a version field will be inconsistent for one of them. That's still pretty niche, do you think it would make sense to rip out this functionality completely?
| if is_local_git_repo_dirty(git_url): | ||
| print_error(f"error: local git clone at {git_url} is dirty") | ||
| return False | ||
| def check_local_git_repo(path: str) -> bool: |
There was a problem hiding this comment.
I struggled with this function ... perhaps it's just the naming and the comment.
- If the path is not a local git repo URL, it returns True. So that means it's a "real" URL like "https://..." or "git:...", right? That's not local, though.
- If it's a directory and there's no git repo in it, it returns True. This is the part I understand.
- But the third check is again just
not is_local_git_repo(). What would this catch?
So perhaps this is check_local_package_sourcetree() or some such?
There was a problem hiding this comment.
Yeah, I cleaned this up a bit. I updated the docstring, and removed the dead code, let me know whether that helped.
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.
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. |
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.
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.