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.

Sign up for GitHub

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

Jump to bottom

bro: 2.5.5 -> 2.6.2 #62695

Closed
wants to merge 1 commit into from
Closed

bro: 2.5.5 -> 2.6.2 #62695

wants to merge 1 commit into from

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Jun 4, 2019

Motivation for this change

Security fix release mentioned at top of notes, FWIW:

https://raw.githubusercontent.com/zeek/zeek/bb979702cf9a2fa67b8d1a1c7f88d0b56c6af104/NEWS

Seems reasonable but if anyone actually familiar can test or otherwise
review that'd be appreciated!

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from pSub June 4, 2019 22:59
@tobim
Copy link
Contributor

tobim commented Jun 5, 2019

Initial Feedback:
A big change that comes with 2.6 is the introduction of the broker communications layer and library, this has some implications:

  • Add caf to buildInputs and "-DCAF_ROOT_DIR=${caf}" to cmakeFlags, otherwise the internal version will be used adding unnecessary compilation time.
  • Add rocksdb to buildInputs to enable brokers store module.
  • The broker python bindings require the ipaddress module which was introduced in python-3.3, so the python parameter should be switched to python3.
  • It would be nice to have a toPythonModule entry in pythonPackages.nix, otherwise the module will be hidden from users.
  • The geoip dependency can be replaced with libmaxminddb, this also enables a requirement for ncurses in one of the tests.

@tobim
Copy link
Contributor

tobim commented Jun 5, 2019

Oh, and you need to add openssl to the buildInputs for caf.

@risicle
Copy link
Contributor

risicle commented Jun 8, 2019

BTW this builds on darwin if you enable the platform. Tested on macos 10.13.

oxzi
oxzi reviewed Jul 15, 2019
View reviewed changes
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this PR and it worked fine for me.

However, the project changed its name from Bro to Zeek and perhaps we should adopt this amendment. An alias for bro could be added for compatibility reasons.

pkgs/applications/networking/ids/bro/default.nix
@@ -2,16 +2,20 @@
, geoip, gperftools, python, swig }:

stdenv.mkDerivation rec {
name = "bro-2.5.5";
name = "bro-2.6.2";

src = fetchurl {
url = "https://www.bro.org/downloads/${name}.tar.gz";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the renaming of the project, this address now forwards to the new domain. So perhaps the domain should already be updated.

url = "https://www.zeek.org/downloads/${name}.tar.gz";

pkgs/applications/networking/ids/bro/default.nix
};

nativeBuildInputs = [ cmake flex bison file ];
buildInputs = [ openssl libpcap perl zlib curl geoip gperftools python swig ];

# Indicate where to install the python bits, since it can't put them in the "usual"
# locations as those paths are read-only.
cmakeFlags = [ "-DPY_MOD_INSTALL_DIR=${placeholder "out"}/${python.sitePackages}" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most Zeek/Bro installation come with the auxiliary tools, like bro-cut. Thus, the "-DINSTALL_AUX_TOOLS:BOOL=true" cmake flag should be added.

@tobim
Copy link
Contributor

tobim commented Jul 24, 2019

The renaming from Bro to Zeek will be put into effect with version 3. So I think we should keep the name until then.

@marsam marsam mentioned this pull request Sep 30, 2019
Merged
10 tasks
@marsam marsam closed this in #70046 Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxzi oxzi oxzi left review comments

@pSub pSub Awaiting requested review from pSub

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dtzWill @tobim @risicle @oxzi