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

Jump to bottom

zmap: init at 2.1.1 #61048

Merged
merged 3 commits into from May 10, 2019
Merged

zmap: init at 2.1.1 #61048

merged 3 commits into from May 10, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented May 6, 2019 •
edited by worldofpeace

Motivation for this change

zmap1 is a fast network scanner for the IPv4 address space. This is
the main package of the ZMap projects, there are further one that will
be packaged soon.

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.

etu
etu approved these changes May 6, 2019
View reviewed changes
Copy link
Contributor

@etu etu left a comment

Choose a reason for hiding this comment

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

Built locally and it seems to work :-)

worldofpeace
worldofpeace suggested changes May 7, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

See above comments. Also darwin build is failing and should be fixed.

@BenBals
Copy link
Contributor

BenBals commented May 8, 2019 •
edited by worldofpeace

Reviewed points
Possible improvements
Comments

@Ma27 Ma27 force-pushed the zmap-package branch 2 times, most recently from e31964f to 589f8ff Compare May 9, 2019 16:16
@Ma27
Copy link
Member Author

Ma27 commented May 9, 2019

@worldofpeace as far as I see I addressed all of the review comments except the one with the config file in/etc. I'm not sure that's the best solution there, do you have a suggestion?:)

@worldofpeace
Copy link
Contributor

worldofpeace commented May 9, 2019 •
edited

@worldofpeace as far as I see I addressed all of the review comments except the one with the config file in/etc. I'm not sure that's the best solution there, do you have a suggestion?:)

@Ma27 You've addressed all the comments except #61048 (comment) #61048 (comment)

I'm think platforms should represent what's supported upstream, unix I think, but we can mark it as broken on darwin since we won't be working for that.

And for the config file situation, it depends on how it's intended to be used in NixOS.
For example, it default reading the config from /etc and not the bundled one in the prefix is useful if there is a nixos module which just adds it to environment.systemPackages and uses environment.etc to link the config from within its output.

@worldofpeace
Copy link
Contributor

Also doing

diff --git a/pkgs/development/tools/parsing/byacc/default.nix b/pkgs/development/tools/parsing/byacc/default.nix
index ba1b8f27e7c..fe8b903180e 100644
--- a/pkgs/development/tools/parsing/byacc/default.nix
+++ b/pkgs/development/tools/parsing/byacc/default.nix
@@ -14,6 +14,10 @@ stdenv.mkDerivation rec {
 
   doCheck = true;
 
+  configureFlags = [
+    "--program-transform-name='s,^,b,'"
+  ];
+
   meta = with stdenv.lib; {
     description = "Berkeley YACC";
     homepage = https://invisible-island.net/byacc/byacc.html;
diff --git a/pkgs/tools/security/zmap/default.nix b/pkgs/tools/security/zmap/default.nix
index 9e509ed3d0e..03395cb492a 100644
--- a/pkgs/tools/security/zmap/default.nix
+++ b/pkgs/tools/security/zmap/default.nix
@@ -13,11 +13,6 @@ stdenv.mkDerivation rec {
     sha256 = "0yaahaiawkjk020hvsb8pndbrk8k10wxkfba1irp12a4sj6rywcs";
   };
 
-  postPatch = ''
-    substituteInPlace src/CMakeLists.txt \
-      --replace "byacc" "yacc"
-  '';
-
   cmakeFlags = [ "-DRESPECT_INSTALL_PREFIX_CONFIG=ON" ];
   dontUseCmakeBuildDir = true;

fixed the issue with byacc not being found for me...

not sure if there should be symlinks for backwards compat

Ma27 added 2 commits May 10, 2019 07:50
@Ma27
byacc: resulting binary should be named byacc
2169fae
@Ma27
zmap: init at 2.1.1
ecafe8a 
zmap[1] is a fast network scanner for the IPv4 address space. This is
the main package of the ZMap projects, there are further one that will
be packaged soon.

[1] https://zmap.io/
@Ma27
Copy link
Member Author

Ma27 commented May 10, 2019

You've addressed all the comments except #61048 (comment) #61048 (comment)

Yeah I've pushed the meta.platform fix before I've seen your comment about marking it broken :)

fixed the issue with byacc not being found for me...

Ahh, I'm afraid I misunderstood the idea behind --program-transform-name, sorry!

And for the config file situation, it depends on how it's intended to be used in NixOS.
For example, it default reading the config from /etc and not the bundled one in the prefix is useful if there is a nixos module which just adds it to environment.systemPackages and uses environment.etc to link the config from within its output.

For example, it default reading the config from /etc and not the bundled one in the prefix is useful if there is a nixos module which just adds it to environment.systemPackages and uses environment.etc to link the config from within its output.

👍

@Ma27
nixos/zmap: init module
3d6fe3d 
The module installs `zmap` globally and links the config files to
`/etc/zmap`, the default location of config files for zmap.

The package provides pretty much a sensitive default, custom configs can
be created like this:

```
{ lib, ... }:
{
  environment.etc."zmap/blacklist.conf" = lib.mkForce {
    text = ''
      # custom zmap blacklist
      0.0.0.0/0
    '';
  };
}
```
@Ma27 Ma27 requested a review from infinisil as a code owner May 10, 2019 06:17
worldofpeace
worldofpeace approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I think It's perfect @Ma27 😄

@worldofpeace worldofpeace merged commit 6c8bb26 into NixOS:master May 10, 2019
@BenBals
Copy link
Contributor

BenBals commented May 10, 2019

I am really happy to see this merged.

@Ma27 Ma27 deleted the zmap-package branch May 10, 2019 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etu etu etu approved these changes

@worldofpeace worldofpeace worldofpeace approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 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
@Ma27 @BenBals @worldofpeace @etu