I believe all requirements for "Trivial" whiteboarding are met. Rpmlint may warn about a few tabs here and there. They will be removed eventually. License is quite clearly a vanilla GPLv3.

FROM: https://fedoraproject.org/wiki/Package_Review_Process
The "Trivial" status is intended to indicate packages which, as an aid to new reviewers, are especially uncomplicated and easy to review. A ticket should not be marked as being trivial unless:

    The package is known to build and a link to a scratch build is included.
    The ticket explains any rpmlint output which is present.
    The spec contains nothing which is unnecessary in modern Fedora (such as BuildRoot:, a %clean section or %defattr).
    The spec is free from excessive or complicated macro usage.
    The spec uses only the least complicated scriptlets which are taken directly from the Packaging:Scriptlets page.
    The package contains no daemons.
    The package is not especially security sensitive.
    The code has undergone a thorough inspection for licensing issues. Anomalies which would be found by licensecheck should be explained. 

In short, this should be reserved only for those tickets which should be easily approachable by someone doing their first package review.

Now builds for EPEL7 (since msgpack does too):

http://koji.fedoraproject.org/koji/taskinfo?taskID=15875241

Hi,
I'm willing to review this package. However, since some time is passed since the initial request, do you plan to update your package to the latest version which seems to be 1.3.1? (or, for some reason, you want to stick with 0.6.x release?!).

Also, maybe the .spec can be modernized a little bit too (e.g. using %make_build)!

Hi,

Thanks very much for your help, this has been in limbo for a while.

Yes, I absolutely plan to update this if you are willing to review.

The plan was to match the version used by ring to facilitate inclusion into fedora:

https://dl.ring.cx/ring-release/tarballs/

In the latest ring release (ring_20170412.1.f0ec025.tar.gz), the opendht version is no longer a release but an intermediate commit between 1.3.0 and 1.3.1 (opendht-09b4e71dc5743d67f35a9604ec93c51c1e109ebe.tar.gz, see git log below).

The fedora way would be to use release 1.3.0 and patch to the proper commit, but then ring seems to patch as well (./daemon/contrib/src/opendht/opendht-uwp.patch).

I need more input from the Ring developers. I will ask Alexandre Viau and Adrien Béraud to comment here.



* 02147b0 (HEAD -> master, tag: 1.3.1, origin/master, origin/HEAD) update to version 1.3.1
* 79e266a crypto: don't build getLongID() with old gnutls
| * 4663903 (origin/feature/c-wrapper) c wrapper: add wrapper foundations
| * 1b4248f log: make use of InfoHash::to_c_str()
|/  
* 9d75ed1 infohash: add to_c_str(), improve toString() performance
* fcca618 infohash: improve getRandom performance
* 73bcebf license cleanup
* 057443e python/tools: update pingpong
* 99f8c29 python: rename bootstrap(sockaddr) to ping
* 4132c63 def: fix typo
*   7f2b5a4 Merge pull request #186 from sim590/search-step-after-announce
|\  
| * 26a7aec dht: fix searchstep not scheduled after announcing
* | 7db0c15 tools: catch exceptions
* | 2fc7cc5 default types: fix uninitialized fields
* | 4970a2a infohash: throw exception with error message
* | 25b6df5 removeExpiredNode: cleanup
* | 7324797 select: cleanup
* | 696ac1b network engine: print error if send fails
* |   3b2e5a1 Merge pull request #185 from savoirfairelinux/feature/pht
|\ \  
| * | b73a50b pht: prevent unneeded use of shared_ptr
|/ /  
* | 70ccbe4 value: remove FieldSelectorDescription
|/  
* 6bb00ab Merge pull request #175 from sim590/argon2-submodule
*   2e27985 Merge pull request #178 from sim590/benchmark-ifname-error
|\  
| * 0a5a609 benchmark: fix invalid ifname error
* |   09b4e71 Merge pull request #182 from atraczyk/master   <----------------------
|\ \  
| * | b407135 ignore MSVC\contrib\build and add argon2 path to project
| * |   c5485a7 Merge branch 'master' of github.com:atraczyk/opendht
| |\ \  
| | * | 9987485 clean up solution
| * | | b7c5d4a clean up solution
|/ / /  
* | | 0aa1c9e argon2: try to fix build on MSVC
* | | 5823e98 rng: add missing header
* | | 3c45e19 Merge pull request #175 from sim590/argon2-submodule
* | | e2ce496 crypto: add Certificate::getCopy()
| | | * 2ac2924 (origin/feature/unittests) implement first simple unit tests
| | | * 8dacf14 crypto: add Certificate::getRawCopy()
| |_|/  
|/| |   
* | | 4540be7 python: add more crypto bindings
* | | 7c85b74 sockaddr: convert port to network/host byte order
* | | fccd569 python: allow to get bound address for any family
* | | 768bfa5 python: add SockAddr support
* | | ff6e385 dhtrunner: add method to bootstrap with a single SockAddr
| |/  
|/|   
* |   c424eb9 Merge pull request #175 from sim590/argon2-submodule
|\ \  
| * | 19c3628 argon2: use as submodule
* | | 501ba77 docker: add missing dep
* | | dc23ee0 docker: update dependencies
* | | e98f06e python: fix scanner
|/ /  
| | * 3b37eb5 (origin/v2) infohash: use 256 bits and SHA256
| | * 75321f5 use libuv for networking and event scheduling
| | * 17f2fbb secure_vector: use mlock to prevent swapping to disc
| | * bc143ad getId compatible with HASH_LEN = 32
| | * 6389800 crypto: use SecureBlob
| |/  
|/|   
* |   358d8bd Merge pull request #176 from sim590/tools-help
|\ \  
| * | c175088 tools: add help -h flag for dhtscanner, dhtchat
* | | 2277a49 crypto: pack/unpack certificate after generation
|/ /  
* | d5427df build: allow to use argon2 system library
* | 36206a8 rng: add missing include
* | ab7d175 network engine: make buffer length depend on HASH_LEN
* | 8025282 crypto: add secure_vector and SecureBlob
* | 95d98f4 rng: add getSeededRandomEngine
* | 4176302 crypto: add PublicKey::toString()
* | b3119cc Update README.md
|/  
* b08b2f1 sockaddr: add missing header and typedef
*   1e7ba56 Merge pull request #174 from sim590/announce-status-fix
|\  
| * faf2319 dht: standardize announce status check
* | 0d66b94 dht: print IPv6 search log
|/  
* 49353e2 (origin/feature/accept_loopback) dht: replace expired node only when bucket is full
* c4d5621 network: treat loopback address as sender address
* aa2227b SockAddr: add utility methods
* 34908d5 network engine: use make_shared
*   4697740 Merge pull request #169 from savoirfairelinux/crypto_checks
|\  
| * 93c5ad7 (origin/crypto_checks) crypto: make Certificate::isCA() more restrictive
|/  
*   03d52e4 Merge pull request #170 from sim590/dht-random-id
|\  
| * 9d83ee1 dht: randomize id in Dht layer
|/  
* 43b181e python: add bindings for network, maintain_storage
* ac65ca5 add shortcut Sp for std::shared_ptr
*   42671be Merge pull request #168 from sim590/announce-op-fixes
|\  
| * efa6930 dht: fix multiple put on a single search segfault
| * 19c47ec dht: correctly tell if value is announced
* | 435cd55 securedht: add getLongId()
* | 10c70eb hash: add string I/O
* | 7da3ff1 hash: add support for SHA256 public key fingerprints
|/  
* a222e30 (tag: 1.3.0) update to version 1.3.0

OK, so I'll wait for the update before doing a formal review, but I'll try to start reviewing before that.

Yes, Fedora allows you to create a package from a git commit rather than a released version (for which you should follow git commit based versioning rules), but I wonder how you should apply ring's opendht patch. If it is going to be applied to opendht upstream, then it is probably fine to include it as a patch with a comment saying that it'll be merged upstream soon. But if it is not going to be included, ... I wonder how we should deal with it.

I suggest to use OpenDHT 1.3.1, that should work fine with Ring.
Also libargon2 should be added as an OpenDHT dependency.

Ring' OpenDHT patch is only used for Windows UWP, so not related to Fedora.

Thanks Adrien. I'll update the spec directly to 1.3.1 then.

This appears to have stalled out.  I'm going to close this to get it off the tracker [0], but feel free to re-open it if/when you're able to pick it back up.

[0]: https://fedoraproject.org/PackageReviewStatus/

Here's an updated version (1.8.2). Anyone willing to review?

Spec URL: https://lef.fedorapeople.org/opendht.spec
SRPM URL: https://lef.fedorapeople.org/opendht-1.8.2-1.fc29.src.rpm
KOJI URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=34050180

I should be able to review this for you.  I haven't run fedora-review on this yet, but just reading the spec file I noticed a few things that can be improved.


Change the Source0 URL to the recommended format [0]:

-Source0:        https://github.com/savoirfairelinux/%{name}/archive/%{version}.tar.gz
+Source0:        https://github.com/savoirfairelinux/%{name}/archive/%{version}/%{name}-%{version}.tar.gz


For libraries detected via pkg-config [1], use the pkgconfig() format [2]:

-BuildRequires:  msgpack-devel
-BuildRequires:  gnutls-devel
-BuildRequires:  nettle-devel
-BuildRequires:  libargon2-devel
+BuildRequires:  pkgconfig(msgpack)
+BuildRequires:  pkgconfig(gnutls)
+BuildRequires:  pkgconfig(nettle)
+BuildRequires:  pkgconfig(libargon2)


The upstream wiki [3] says the Cython build requirement is for the optional Python bindings, but I don't see those listed in your %files.  Also the upstream setup.py.in file [4] indicates this is for Python 3, not 2, so you need to build require python3-Cython.


Wrap the lines of the description at 80 characters [5].


Don't use the Group tag [6].


The first line of the autogen.sh script [7] is to run `git submodule update --init`.  You don't have git as a build requirement, so the build logs have the error `BUILDSTDERR: ./autogen.sh: line 1: git: command not found`.  But more importantly, packages aren't allowed to download from the internet during the build.  The submodule is for argon2, which you already build require, so skip autogen.sh and just run autoreconf directly.


Use %make_build and %make_install [8].


Remove the ldconfig scriptlets, RPM takes care of this automatically now [9].  Unless you intend to also package this for EPEL, in which case use %ldconfig_scriptlets (which is a no-op on Fedora).


Remove `%{!?_licensedir:%global license %doc}`, %license is defined in all Fedora and EPEL branches now.


Your %changelog needs at least one item for the 2018 entry, such as `- Latest upstream`.  Alternatively you can just delete the 2016 line and use the initial packaging item for the 2018 entry.


Upstream has a test suite, try to add a %check section and run it [10].


[0]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags
[1]: https://github.com/savoirfairelinux/opendht/blob/1.8.2/configure.ac#L128-L130
[2]: https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/
[3]: https://github.com/savoirfairelinux/opendht/wiki/Build-the-library
[4]: https://github.com/savoirfairelinux/opendht/blob/1.8.2/python/setup.py.in#L41-L45
[5]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_summary_and_description
[6]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections
[7]: https://github.com/savoirfairelinux/opendht/blob/1.8.2/autogen.sh#L1
[8]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_parallel_make
[9]: https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets
[10]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites

Thank you so much! As you can see, I am a bit rusty on this, but I have updated the spec according to your comment.

Spec URL: https://lef.fedorapeople.org/opendht.spec
SRPM URL: https://lef.fedorapeople.org/opendht-1.8.2-1.fc29.src.rpm
KOJI URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=34053873

No worries, you're doing great so far.  Thanks for making those adjustments.

It doesn't look like the tests are actually getting run.  From your scratch build log:

+ make check
Making check in src
make[1]: Entering directory '/builddir/build/BUILD/opendht-1.8.2/src'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/builddir/build/BUILD/opendht-1.8.2/src'
Making check in tools
make[1]: Entering directory '/builddir/build/BUILD/opendht-1.8.2/tools'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/builddir/build/BUILD/opendht-1.8.2/tools'
Making check in python
make[1]: Entering directory '/builddir/build/BUILD/opendht-1.8.2/python'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/builddir/build/BUILD/opendht-1.8.2/python'
make[1]: Entering directory '/builddir/build/BUILD/opendht-1.8.2'
make[1]: Leaving directory '/builddir/build/BUILD/opendht-1.8.2'

Upstream says you'll need pkgconfig(cppunit) for the tests [0], but I'm not sure if that being present will by itself enable the tests.  I'm not an expert on autoconf stuff, but if I understand that configure.ac right, I think running configure with `--enable-tests` will turn it on.  Try that and see if the %check output looks different.

One more small thing I noticed, the license headers in the source say "either version 3 of the License, or (at your option) any later version", so the License field needs to be GPLv3+.

Since you included a comment about EPEL7, I thought I'd mention that upstream claims to need at least GCC 5.2.  That will be a problem a EL7 only has 4.8.5.  I've heard that EPEL7 packages can now use devtoolset to get a newer compiler, but I don't know much about the specifics.  On that note, you may want to include the minimum versions of the build requirements listed in the README.

[0]: https://github.com/savoirfairelinux/opendht/blob/1.8.2/configure.ac#L123

Thanks. It seems restbed is required for the tests and that does not seem to be yet packaged in fedora. It's one of the optional dependencies.
If Adrien is still reading this, maybe he can give us some input.

And yes, RHEL7 is getting old. I'll worry about EPEL7 some other time, if ever.
The spec is updated and I triggered a new scratch build without tests: https://koji.fedoraproject.org/koji/taskinfo?taskID=34069441

That's fine about the test suite, it's understandable to skip it if the build requirements to run it are missing.  Try to enable it in the future once you can.

I ran fedora-review on this now, and it has highlighted a few other issues.


> [ ]: Package contains systemd file(s) if in need.

Upstream has unit files for dhtnode and dhtcluster, please include them and their relevant config files (see my note about cmake below).


> [ ]: Latest version is packaged.

Upstream is chugging right along and has released 1.9.0.


> opendht.x86_64: E: explicit-lib-dependency libargon2

RPM automatically adds dependencies for linked libraries it detects, so you should be able to just remove these:

-Requires:       msgpack
-Requires:       gnutls
-Requires:       libargon2


> opendht.x86_64: W: no-manual-page-for-binary dhtchat
> opendht.x86_64: W: no-manual-page-for-binary dhtnode
> opendht.x86_64: W: no-manual-page-for-binary dhtscanner

Upstream has a man page for dhtnode, please include it (see my note about cmake below).  There isn't man pages for the other two, don't worry about them.


> opendht.src:15: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 15)

Use one or the other.  Most packagers stick with just spaces.


> opendht.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopendht.so.0.0.0 /lib64/librt.so.1
> opendht.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopendht.so.0.0.0 /lib64/libdl.so.2

Try to fix these (see my note about cmake below).

https://fedoraproject.org/wiki/Common_Rpmlint_issues#unused-direct-shlib-dependency


Something else I noticed that I didn't before was your use of globs in %files.  You can use globs, but try to be more explicit.  Here's how I would do it.

-%{_libdir}/*.so.*
+%{_libdir}/libopendht.so.0*

-%{_includedir}/*
-%{_libdir}/*.so
+%{_includedir}/opendht*
+%{_libdir}/libopendht.so

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files


Speaking of the libraries, I thought it was strange that the soname was 0.0.0.  If you compile with autotools, you get libopendht.so.0.0.0.  But if you compile with cmake, you get libopendht.so.1.8.2.  This was reported upstream in the past, fixed, then regressed.

https://github.com/savoirfairelinux/opendht/issues/75
https://github.com/savoirfairelinux/opendht/pull/95#issuecomment-481768325

I can't even get 1.9.0 to compile with autotools, so I suggest just switching to cmake.
This will have some other benefits too, such as installing the systemd files and man pages.  It also makes the unused-direct-shlib-dependency rpmlint warning go away.  Here's the changes I would make.

+BuildRequires:  cmake

(in the devel subpackage)
+Requires:       cmake-filesystem

(in %build)
-autoreconf --install --verbose -Wall
-%configure --disable-static
+%cmake -DOPENDHT_STATIC=OFF -DOPENDHT_SYSTEMD=ON .

(in %install)
-find $RPM_BUILD_ROOT -name '*.la' -delete

(in %files)
+%config %{_sysconfdir}/dhtnode.conf
+%{_unitdir}/dhtnode.service
+%{_mandir}/man1/dhtnode.1*

(in %files devel)
+%{_libdir}/cmake/opendht


Last suggestion, have you considered splitting the files out further?  It's fine as is, but it might be beneficial to end users to have a more minimal install of just the tool they want.  I was thinking of separate packages for:

- dhtchat (command)
- dhtnode (command, systemd unit, systemd config, man page)
- dhtscanner (command)
- libopendht (library)
- opendht-devel (devel files)

I'm not familiar enough with this software to say which should depend on each other (or if this approach even makes sense), so feel free to disagree here.  I just think in the interest of minimalism, another package should be able to require libopendht without pulling in CLI tools or systemd units.  You could also put all the CLI tools in one subpackage, separate from the library, such as opendht-tools.

Upstream fixed that soname 0.0.0 problem.

https://github.com/savoirfairelinux/opendht/issues/75#issuecomment-484248050

If you'd like to stick with autotools, either wait for that commit to be included in a release or add it as a patch.  Or just switch to cmake as previously described.

Hi all,

Can I help with this at all? I'm about to start using OpenDHT in my project SentryPeer (already on Bugzilla):

 https://bugzilla.redhat.com/show_bug.cgi?id=2026516
 https://copr.fedorainfracloud.org/coprs/ghenry/SentryPeer/build/3251825/
 https://github.com/SentryPeer/SentryPeer

I'm also using Zyre, which would be another good one to get in Fedora:

 https://github.com/zeromq/zyre (which needs zeromq and czmq, both already in Fedora).

What's needed to get OpenDHT back on track?

Thanks,
Gavin.

Someone needs to update the spec file to address the issues identified in comment 14, especially updating it to the latest version.  If Benjamin can do it, we can keep the work in this bug.  If someone else takes it over, they can start a new bugzilla and close this one as a duplicate of it.  This is because once the package is approved, the person requesting the dist-git repo must match the reporter field on the approved bug.

Benjamin, are you still interested in this?

I've just finished packaging this for Alpine Linux and Homebrew and am using it now in SentryPeer. I know it much better than I did when I added my comment to this ticket.

Happy to help. 

Thanks.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.

Can I take this then? I need it for my Fedora sentrypeer reqs. Thanks

Yes, anyone can submit a new review for opendht in a new bug, as I mentioned in comment 17.