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

pythonPackages.easysnmp: init at 0.2.5; python3Packages.poster3: init at 0.8.1 #66645

Merged
merged 3 commits into from Nov 5, 2019

Conversation

WhittlesJr
Copy link
Contributor

Motivation for this change

Add easysnmp and poster3.

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @lheckemann: whom I believe to be sphalerite on IRC, with whom I talked about this briefly and obliquely on IRC on 8/06/2019.

@WhittlesJr WhittlesJr requested a review from FRidh as a code owner August 14, 2019 21:11
@WhittlesJr WhittlesJr changed the title pythonPackagse.easysnmp: init at 0.2.5; python3Packages.poster3: init at 0.8.1 pythonPackages.easysnmp: init at 0.2.5; python3Packages.poster3: init at 0.8.1 Aug 14, 2019
Copy link
Member

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

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

You believe correctly ;)

Looks good overall, just a few changes.

pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/easysnmp.patch Outdated Show resolved Hide resolved
@lheckemann
Copy link
Member

@GrahamcOfBorg build python3Packages.easysnmp python3Packages.poster3

pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/poster3/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/poster3/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/poster3/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/poster3/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
@WhittlesJr
Copy link
Contributor Author

I believe I've implemented all the feedback. Is the next step to squash and rebase onto master?

@jonringer
Copy link
Contributor

jonringer commented Aug 19, 2019

yes, please rebase so that you have 3 commits, 1 for maintainer entry, and 1 for each package. And it wouldn't be a bad time to rebase off master since this has been open a little while.

EDIT: please rebase so that you have 3 commits in this PR

@WhittlesJr
Copy link
Contributor Author

Oh, I was told to have three commits in one PR. I think that was @lheckemann's advice...

@WhittlesJr
Copy link
Contributor Author

Oops hold on, I missed something

@jonringer
Copy link
Contributor

sorry, my terms got mixed up, 3 commits in one PR

@WhittlesJr
Copy link
Contributor Author

OK, all set, I think

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
leaf packages

a little concerned whether the easysnmp package needs the net_snmp bin at runtime, but I don't have a good way to verify if thats the case.

@jonringer
Copy link
Contributor

jonringer commented Aug 19, 2019

@WhittlesJr if you addressed a comment, you can resolve the individual comment windows. :)

Should clean up chat quite a bit

@WhittlesJr
Copy link
Contributor Author

@jonringer, I believe net_snmp is needed at runtime. Does it need to be propagatedBuildInputs instead of buildInputs? My usage of it has been working as written.

@jonringer
Copy link
Contributor

jonringer commented Aug 20, 2019

I think in most practical cases, where a user references this package directly, it should work. (It might work as a dependency of another? idk)

The use case I was thinking of, is when a different package references this, then the path might not have the net_snmp binaries. Only way around this (that i know of) would be to patch the source so that it uniquely finds net_snmp. Example patch for the package pyproj. This way, the package doesn't go to path or other mechanisms to find the binary, it will just return the full path exactly to what it's looking for, regardless of environment.

@WhittlesJr
Copy link
Contributor Author

Hmm, after grepping around, I don't see it shelling out to any net-snmp binaries at runtime. I suspect we're probably good with what we have here.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM (Nice!)
leaf packages

https://github.com/NixOS/nixpkgs/pull/66645
2 package were build:
python27Packages.easysnmp python37Packages.poster3

@lheckemann
Copy link
Member

One more nit: https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix#L23 was just introduced recently, could you add your github id to maintainers.nix as well? Then I'll merge :)

@WhittlesJr
Copy link
Contributor Author

@lheckemann, how does that look? I also alphabetized my entry in maintainer-list.nix.

@@ -1608,6 +1608,11 @@ in {

dugong = callPackage ../development/python-modules/dugong {};

easysnmp = callPackage ../development/python-modules/easysnmp {
openssl = pkgs.openssl;
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary, pythonPackages will fill missing stuff in from pkgs.

Copy link
Contributor

Choose a reason for hiding this comment

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

i recommended this, in case there's a package added to python-modules of that name. I feel like it's worth-while future-proofing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me

Copy link
Member

Choose a reason for hiding this comment

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

Still 👎 on this. I'd be against adding something called openssl or net_snmp to python-packages anyway, and there are plenty of other packages using non-python deps without them being threaded through specifically. (also, if you really want to, you can rephrase this as inherit (pkgs) openssl net_snmp).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do see lots of examples of the inherit (pkgs) pattern. Shall we use it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's just because I'm not a big fan of bringing in a entire package sets. I wonder what @FRidh has to say on pkgs vs per-package imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased on master again. I'd like to do whatever we need to do to get this merged. Sounds like there's disagreement on which way to go, so we need an authoritative decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinging @jonringer, @lheckemann, and @FRidh. I need to know which way to go with this change. Please advise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still in favor of the current state. I'm not a committer, so I can't merge :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonringer, do you know who else we might try pinging?

@lheckemann
Copy link
Member

Sorry, one more change! Other than that it does look good 😅

@WhittlesJr WhittlesJr force-pushed the python-easysnmp-and-poster3 branch 3 times, most recently from ee0992f to 29ac392 Compare September 13, 2019 17:12
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

I'm not sure about adding easysnmp to nixpkgs, it looks like it's not maintained, and hasn't seen any attention for 13 months, https://github.com/kamakazikamikaze/easysnmp

pkgs/development/python-modules/easysnmp/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/poster3/default.nix Outdated Show resolved Hide resolved
@WhittlesJr
Copy link
Contributor Author

That would be pretty discouraging... it's still actively used, if not maintained.

@lheckemann lheckemann merged commit 19fb942 into NixOS:master Nov 5, 2019
@lheckemann
Copy link
Member

Sorry for the sluggishness!

@WhittlesJr
Copy link
Contributor Author

Thank you!

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Nov 5, 2019
…ster3

pythonPackages.easysnmp: init at 0.2.5; python3Packages.poster3: init at 0.8.1

(cherry picked from commit 19fb942)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants