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

ndn-cpp: new pkg #48760

Closed
wants to merge 1 commit into from
Closed

ndn-cpp: new pkg #48760

wants to merge 1 commit into from

Conversation

sjmackenzie
Copy link
Contributor

Motivation for this change
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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@JohnAZoidberg
Copy link
Member

Looks good! You could add the optional dependencies as well, they're described here.

If you think they bloat the package too much you could make them optional like it's done here with a dependency on OpenSSL.

@sjmackenzie
Copy link
Contributor Author

Yeah we've tried omitting openssl and it doesn't build.

@JohnAZoidberg
Copy link
Member

That's not what I mean. Sorry, OpenSSL was a bad example.
The other project I referenced can make OpenSSL optional but it's required for this library.
What I mean is that ndn-cpp lists some optional dependencies in the first link I referenced:

Optional: libsqlite3 (for key storage)
Optional: OSX Security framework (for key storage)
Optional: Protobuf (for the ProtobufTlv converter and ChronoSync)
Optional: log4cxx (for debugging and log output)
Optional: Doxygen (to make documentation)
Optional: Boost (min version 1.48) with asio (for ThreadsafeFace and async I/O)

We could either build with all of those or make them configurable in the nix file. I referenced the other project so that you can see how it could be done.

FRidh
FRidh requested changes Oct 21, 2018
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Commit title is not according to the guidelines

pkgs/development/libraries/ndn-cpp/default.nix Outdated Show resolved Hide resolved
@sjmackenzie
Copy link
Contributor Author

@JohnAZoidberg as nice as it would be to have those configurable, things like boost etc get annoying. This is a minimal + mandatory configuration. If others want it configurable, they're welcome to patch the code. If you find this unacceptable I'll close this patch.

@JohnAZoidberg
Copy link
Member

That was just a suggestion. I have no authority, I cannot review nor merge your changes.
In my personal opinion it's better if we the packages support more dependencies - because you know about the package and how to build it, a user might not know that but still wants to use the optional dependencies.

I don't think we should close this, it's totally fine without all dependencies.
If a maintainer thinks you packaged it well then by all means have them merge it.

Sorry that it's not merged yet - I'm sure it's just because there are so many pull requests.

aanderse
aanderse requested changes Apr 9, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Please make the 2 minor changes I have requested and we will get this merged ASAP.

pkgs/development/libraries/ndn-cpp/default.nix
version = "v0.15";
in
stdenv.mkDerivation {
name = "ndn-cpp-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

The new way to set the name is like so:

pname = "ndn-cpp";
version = "v0.15";

With this name will automatically be set to ndn-cpp-v0.15.

Copy link
Member

Choose a reason for hiding this comment

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

0.17 is now out. @sjmackenzie are you still interested in packaging this?

pkgs/development/libraries/ndn-cpp/default.nix
@@ -0,0 +1,25 @@
{ stdenv, fetchFromGitHub, openssl, doxygen, zlib, pkgconfig, protobuf }:
let
version = "v0.15";
Copy link
Member

Choose a reason for hiding this comment

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

Please bump version to 0.16.

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'm actually rewriting it in Rust. I'll close this PR, I won't be maintaining it.

JohnAZoidberg
JohnAZoidberg requested changes Nov 11, 2019
View reviewed changes
pkgs/development/libraries/ndn-cpp/default.nix
version = "v0.15";
in
stdenv.mkDerivation {
name = "ndn-cpp-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

0.17 is now out. @sjmackenzie are you still interested in packaging this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh requested changes

@JohnAZoidberg JohnAZoidberg JohnAZoidberg requested changes

@aanderse aanderse aanderse requested changes

Assignees
No one assigned
Labels
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
5 participants
@sjmackenzie @JohnAZoidberg @FRidh @aanderse @GrahamcOfBorg