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

pypcap: init at 1.1.6 #30226

Merged
merged 1 commit into from Oct 12, 2017
Merged

pypcap: init at 1.1.6 #30226

merged 1 commit into from Oct 12, 2017

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Oct 8, 2017 •
edited

Motivation for this change

This PR adds the python libpcap module.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested execution of all binary files (usually in ./result/bin/)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Fits CONTRIBUTING.md.

@oxzi oxzi requested a review from FRidh as a code owner October 8, 2017 17:20
vyp
vyp reviewed Oct 8, 2017
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
homepage = https://github.com/pynetwork/pypcap;
description = "Simplified object-oriented Python wrapper for libpcap";
license = licenses.bsd3;
};
Copy link
Member

Choose a reason for hiding this comment

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

A maintainer must be set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, forgot this one. Should be fixed now.

FRidh
FRidh requested changes Oct 8, 2017
View reviewed changes
pkgs/top-level/python-packages.nix Outdated
@@ -17594,6 +17594,49 @@ in {
};
};

pypcap = buildPythonPackage rec {
Copy link
Member

Choose a reason for hiding this comment

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

see the header of this file. This expression shouldn't be here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I simply was overwhelmed by this 26k lines long file and ignored the head. Should be fixed now.

pkgs/top-level/python-packages.nix Outdated

buildInputs = [ pkgs.libpcap ];

doCheck = false;
Copy link
Member

Choose a reason for hiding this comment

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

Why? Include a comment.

@oxzi oxzi force-pushed the pypcap-1.1.6 branch 2 times, most recently from 496780e to e1cbcbe Compare October 8, 2017 19:39
FRidh
FRidh reviewed Oct 9, 2017
View reviewed changes
pkgs/development/python-modules/pypcap/default.nix Outdated
@@ -0,0 +1,42 @@
{ stdenv, lib, buildPythonPackage, fetchPypi, isPy3k, pythonPackages, pkgs }:
Copy link
Member

Choose a reason for hiding this comment

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

pass in in individual dependencies, so dpkt and libpcap, instead of pythonPackages and pkgs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your feedback. I applied the requested changes.

@oxzi oxzi force-pushed the pypcap-1.1.6 branch 2 times, most recently from 3f9cef7 to b543c94 Compare October 9, 2017 16:36
FRidh
FRidh requested changes Oct 9, 2017
View reviewed changes
pkgs/development/python-modules/pypcap/default.nix Outdated
@@ -0,0 +1,42 @@
{ stdenv, lib, pkgs, buildPythonPackage, fetchPypi, isPy3k, libpcap, dpkt }:
Copy link
Member

Choose a reason for hiding this comment

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

pkgs is left

pkgs/development/python-modules/pypcap/default.nix Outdated
patches = [
# The default setup.py searchs for pcap.h in a static list of default
# folders. So we have to add the path to libpcap in the nix-store.
(pkgs.writeText "libpcap-path.patch"
Copy link
Member

Choose a reason for hiding this comment

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

and here

@oxzi
Copy link
Member Author

oxzi commented Oct 12, 2017

I just wanted to ask if the last changes are fulfilling the expectations. Is this PR now able to be merged?
CC: @FRidh

@FRidh
Copy link
Member

FRidh commented Oct 12, 2017

@geistesk are the tests found and run? Does it build on the different interpreter versions?

@oxzi
Copy link
Member Author

oxzi commented Oct 12, 2017

@FRidh The tests should succeed. Before adding the dpkt-packages the tests failed and this dependency is just required for the testsniff.py-file.
The project just works with Python 2.7 atm as one can read in pynetwork/pypcap/issues/24. That's why I disabled Python 3 in line 7.
I'm using this package in a local repository for over a week and it works fine for me. Otherwise I hand't contributed it…

@FRidh FRidh merged commit 959a5b2 into NixOS:master Oct 12, 2017
@oxzi oxzi deleted the pypcap-1.1.6 branch October 12, 2017 17:43
oxzi added a commit to umr-ds/traffic-ui that referenced this pull request Oct 13, 2017
@oxzi
Updated shell.nix
6a307de 
Now executes the tests; based on NixOS/nixpkgs#30226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyp vyp vyp left review comments

@FRidh FRidh FRidh requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oxzi @FRidh @vyp