-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider installed zeek and zkg versions when finding best version tag #99
Comments
I feel like this is generally a good idea, with one bit but. I think we should make it very obvious (much more obvious than in the current output) that the version that is being installed is not the up-to-date version. We also should consider what this means from a security point of view. Consider someone writing a protocol analyzer that works for Zeek version A. Then a new version of Zeek comes out - and the analyzer gets updated. Then a security issue gets found and fixed. If you install the package with an old version of Zeek - you, in this case, will get a package installation with a known vulnerability, that will probably never get updated. I know that this might seem a bit far fetched - but I actually see a pretty good chance that something along these lines will eventually happen. |
Yeah Johanna, agree to all. |
Thanks, seems like a neat idea to me, too, and I'd also try to make what's happening clearer to user. An alternate way to do that may even be to avoid any change to preferred-version/dependency-analysis code: instead the logic on the
Hard to install old/outdated packages or get them by mashing "yes" without conscious thought that way. Not that I know what level of convenience/safety tradeoff is most efficient, but think that still addresses problems presented:
|
Nice, that would work for me! It's an extra step for the user, but it's reasonable for us to be concerned, and I also like that this is a bit of a nudge to upgrade Zeek (or zkg). On the implementation side it would be nice if the dependency validation code indicated explicitly that failure is due to Zeek/zkg versions ... I believe it currently would tell us only by parsing the error message. But that won't be hard to add — for example, we could return the offending node(s) in some form. An aside on adding more functionality to the toplevel |
I just realized there's also |
Doing that error message + "here's what to do if you're sure" sounds good to me too. The one other idea I'd have it leaving it to the package author: we could enable them to declare in the meta data of the most recent version what older versions to fall back to in which case (i.e., some form of dependency spec saying |
When a package has version tags, installation of that package without requesting a specific version or branch causes
zkg
to pick the numerically greatest version tag (based on semver ordering). That's definitely reasonable, but we currently don't deviate from that version even if it's incompatible with the installed Zeek orzkg
version.Over time, it's pretty likely that newer version tags are compatible only with newer Zeek versions. If a package doesn't make those dependencies explicit, there's not a whole lot we can do, but when it does, I'm thinking it would be reasonable to step backward through the version history to find one that's compatible with what's locally available.
Consider Community ID: its latest version (3.2.0) works with any Zeek >= 3.2, and it has older tags that work with older Zeeks. If I have a recent Zeek, all is well, but with an older Zeek this happens:
To fix this, you have to know which version tag to pick. It's documented in the README, but the user needs to figure it out. I put together a prototype that walks back through the tags, here ... with it I get:
It first considered the latest tag, 3.2.0, which fails the Zeek dependency, and then moved on to 3.1.0, which is marked compatible with Zeek 3.1.5, currently installed.
I'm definitely not sure this is the right way to do it since the code has a few spots where it picks the greatest version number and the dependency code is a bit tricky. I was mainly curious to see if other hiccups would come up, but it seems manageable.
Thoughts?
The text was updated successfully, but these errors were encountered: