Skip to content
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

Remove Bro-era constructs #119

Open
ckreibich opened this issue Oct 11, 2021 · 8 comments
Open

Remove Bro-era constructs #119

ckreibich opened this issue Oct 11, 2021 · 8 comments

Comments

@ckreibich
Copy link
Member

zkg still supports various Bro-era environment variables, .bro files, bro-config invocations, etc that we should remove at this point. (This'll be a reason to jump to 3.0., so a good time to consider other updates that introduce incompatibilities.)

@timwoj
Copy link
Member

timwoj commented Jun 20, 2023

Should we still support bro-pkg.meta?

@ckreibich
Copy link
Member Author

Good point, yeah — I think it's better to keep. In the standard package source, 47 out of 226 packages still use bro-pkg.meta. At a glance at least some of these should run fine still, so it seems prudent.

@JustinAzoff
Copy link

@ckreibich Did you check if packages only had a bro-pkg.meta or both a bro-pkg.meta and a zkg.meta? I know a few of mine probably have both, and the bro-pkg.meta was just never removed.

@timwoj
Copy link
Member

timwoj commented Jun 23, 2023

@ckreibich Did you check if packages only had a bro-pkg.meta or both a bro-pkg.meta and a zkg.meta? I know a few of mine probably have both, and the bro-pkg.meta was just never removed.

I have a recent clone of all of the package repos and of the 48 packages with a bro-pkg.meta, only 4 of them have a zkg.meta as well.

@awelzel
Copy link
Contributor

awelzel commented Jun 23, 2023

I have a recent clone of all of the package repos and of the 48 packages with a bro-pkg.meta, only 4 of them have a zkg.meta as well.

More numbers: Of the 47 packages with bro-pkg.meta, 40 have no traces of *.zeek files, so these are unlikely to work.

I'd think dropping bro-pkg.meta as the majority needs a bigger revamp seemingly.

@timwoj
Copy link
Member

timwoj commented Jun 23, 2023

I'd think dropping bro-pkg.meta as the majority needs a bigger revamp seemingly.

@ckreibich and I chatted about this during a call yesterday actually. At some point we're going to have to rip the bandaid off for pushing packages forward into the current era. This may require some concerted effort by the Zeek team ourselves to do things like getting rid of bro-pkg.meta, updating to the new plugin CMake API, getting rid of BRO variable references, etc. There's a number of packages out there that are seemingly unmaintained, but I'd be willing to bet that if we opened PRs against them to do these things they would be accepted.

@timwoj
Copy link
Member

timwoj commented Jun 23, 2023

Another leftover is the __bro_plugin__ file that we generate as part of plugin builds.

@ckreibich
Copy link
Member Author

Alright ... I came out in favor of leaving bro-pkg.meta in there because its largely an external issue (we don't control those packages, as opposed to stuff like us mentioning bro-pkg.meta in our own docstrings/warnings/etc, which I'm all for ripping out asap) and it's unobtrusive in the codebase — it's really just manager._pick_metadata_file() using LEGACY_METADATA_FILENAME. But it's also true that if we rip out Bro stuff broadly it seems weird to keep just that.

We haven't discussed a deprecation cycle for zkg. The only instance I recall where a somewhat similar decision came up in the past was when we switched bro-pkg.index to zkg.index in the package source, which iirc we handled ad-hoc — an announcement on Slack ahead of time and then we renamed instances in the packages repo, but we made no change in the zkg code.

Since zkg is packaged with Zeek it would seem reasonable to me to align the deprecation cycle, throwing out deprecations in whatever version ships with Zeek x.1, and warning about them in other versions. It looks to me like we still need to wrap up a zkg release that ships with Zeek 6 — that could be 2.14.0, though we'd need to get deprecation warnings in quite quickly (which seems doable). We could then ship a zkg 3.0.0 with 6.1, throwing out the Bro era in it.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants