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
pythonPackages.easysnmp: init at 0.2.5; python3Packages.poster3: init at 0.8.1 #66645
Conversation
61853c0
to
ddd53cb
Compare
There was a problem hiding this 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.
@GrahamcOfBorg build python3Packages.easysnmp python3Packages.poster3 |
I believe I've implemented all the feedback. Is the next step to squash and rebase onto master? |
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 |
Oh, I was told to have three commits in one PR. I think that was @lheckemann's advice... |
161288e
to
5fba144
Compare
Oops hold on, I missed something |
sorry, my terms got mixed up, 3 commits in one PR |
5fba144
to
4c32283
Compare
OK, all set, I think |
There was a problem hiding this 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.
@WhittlesJr if you addressed a comment, you can resolve the individual comment windows. :) Should clean up chat quite a bit |
@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. |
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. |
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. |
There was a problem hiding this 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
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 :) |
4c32283
to
620f15d
Compare
@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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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?
Sorry, one more change! Other than that it does look good 😅 |
ee0992f
to
29ac392
Compare
There was a problem hiding this 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
That would be pretty discouraging... it's still actively used, if not maintained. |
9017d2d
to
8abfbb9
Compare
Sorry for the sluggishness! |
Thank you! |
…ster3 pythonPackages.easysnmp: init at 0.2.5; python3Packages.poster3: init at 0.8.1 (cherry picked from commit 19fb942)
Motivation for this change
Add easysnmp and poster3.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)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.