Hide Forgot
Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec SRPM URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.1.alpha1.fc23.src.rpm Description: dnsdist is a highly DNS-, DoS- and abuse-aware loadbalancer. Its goal in life is to route traffic to the best server, delivering top performance to legitimate users while shunting or blocking abusive traffic. Fedora Account System Username: tjikkun
Thanks your for you spec! > #%license COPYING License is mandatory for all new packages [1]. Your ticket has been fixed in the upstream, please enable https://github.com/PowerDNS/pdns/pull/3202 [1]: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text -------------- > BuildRequires: systemd-units => BuildRequires: systemd systemd-units has been merged into systemd package [2]. [2]: https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations -------------- > # install systemd unit file > %{__install} -D -p -m 644 contrib/%{name}.service %{buildroot}%{_unitdir}/%{name}.service + Requires(post): systemd + Requires(preun): systemd + Requires(postun): systemd + + %post + %systemd_post %{name}.service + + %preun + %systemd_preun %{name}.service + + %postun + %systemd_postun_with_restart %{name}.service [3] https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#New_Packages -------------- > %{_mandir}/man1/%{name}.1.gz > When installing man pages, note that they should be installed uncompressed as the build system will compress them as needed. The compression method may change, so it is important to reference the pages in the %files section with a pattern that takes this into account [4] [4]: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > /usr/bin/install -c -m 644 dnsdist.1 '/builddir/build/BUILDROOT/dnsdist-1.0.0-0.1.alpha1.fc24.x86_64/usr/share/man/man1' -------------- dnsdist-1.0.0-alpha1/html/js/jquery-1.8.3.min.js dnsdist-1.0.0-alpha1/html/js/jsrender.js dnsdist-1.0.0-alpha1/html/js/moment.min.js dnsdist-1.0.0-alpha1/html/js/purl.js dnsdist-1.0.0-alpha1/html/js/rickshaw.min.js dnsdist-1.0.0-alpha1/html/js/underscore-min.js ... Obfuscated (=compiled) JavaScript looks like a binary for me: > When you encounter prebuilt binaries in a package you MUST: > > Remove all pre-built program binaries and program libraries in %prep prior to the building of the package. > Ask upstream to remove the binaries in their next release. [5] [5] https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries I'm not so experienced with Fedora packaging yet, but some other distros (you know) blame for all these *.min.js VERY strongly, so upstream might have problems with other packages as well. Please correct me if I'm wrong. -------------- %dir %{_sysconfdir}/%{name}/ It would be nice to include an example of configuration file. -------------- Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in dnsdist-debuginfo -------------- > -I/builddir/build/BUILD/dnsdist-1.0.0-alpha1/ext/mbedtls/include mbedtls.i686 : Light-weight cryptographic and SSL/TLS library mbedtls.x86_64 : Light-weight cryptographic and SSL/TLS library mbedtls-utils.x86_64 : Utilities for mbedtls mbedtls-static.i686 : Static files for mbedtls mbedtls-static.x86_64 : Static files for mbedtls mbedtls-devel.i686 : Development files for mbedtls mbedtls-devel.x86_64 : Development files for mbedtls mbedtls-doc.noarch : Documentation files for mbedtls -------------- ^I--disable-silent-rules \$ --enable-dnscrypt \$ --enable-libsodium \$ ^I--with-lua \$ (`:set list` in vim) -------------- > Summary: A highly DNS-, DoS- and abuse-aware loadbalancer Remove the "A" article from Summary, it looks better in listings without. It is not mandatory and is not mentioned in the guidelines, but I was noticed about that couple times by Zbigniew Jędrzejewski-Szmek. -------------- Please try to use koji to ensure that package compiles on all supported architectures: koji build --scratch rawhide pdns-4.0.0-0.1.alpha1.fc24.src.rpm -------------- What the difference between https://github.com/PowerDNS/pdns and this software? Shouldn't dnsdist to be added as a subpackage of existing pdns package? -------------- It seems that pdns packages also uses yahttp. Probably yahttp needs their own package. -------------- License and unclear relation with existing pdns package are major problems here.
(In reply to Roman Tsisyk from comment #1) > Thanks your for you spec! Thanks for reviewing! > > > #%license COPYING > > License is mandatory for all new packages [1]. > Your ticket has been fixed in the upstream, please enable > https://github.com/PowerDNS/pdns/pull/3202 > > [1]: > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text > Will fix this with the release of second alpha, expected soon > -------------- > > > BuildRequires: systemd-units > > => BuildRequires: systemd > > systemd-units has been merged into systemd package [2]. > > [2]: https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations Yes but for EPEL 6 you need systemd-units right? So that works on al releases. If I need to make it conditional I can of course. > > -------------- > > > # install systemd unit file > > %{__install} -D -p -m 644 contrib/%{name}.service %{buildroot}%{_unitdir}/%{name}.service > > + Requires(post): systemd > + Requires(preun): systemd > + Requires(postun): systemd > + > + %post > + %systemd_post %{name}.service > + > + %preun > + %systemd_preun %{name}.service > + > + %postun > + %systemd_postun_with_restart %{name}.service > > [3] https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#New_Packages > Will fix > -------------- > > > %{_mandir}/man1/%{name}.1.gz > > > When installing man pages, note that they should be installed uncompressed as the build system will compress them as needed. The compression method may change, so it is important to reference the pages in the %files section with a pattern that takes this into account [4] > > [4]: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > > > /usr/bin/install -c -m 644 dnsdist.1 '/builddir/build/BUILDROOT/dnsdist-1.0.0-0.1.alpha1.fc24.x86_64/usr/share/man/man1' > Ok, will fix > -------------- > > dnsdist-1.0.0-alpha1/html/js/jquery-1.8.3.min.js > dnsdist-1.0.0-alpha1/html/js/jsrender.js > dnsdist-1.0.0-alpha1/html/js/moment.min.js > dnsdist-1.0.0-alpha1/html/js/purl.js > dnsdist-1.0.0-alpha1/html/js/rickshaw.min.js > dnsdist-1.0.0-alpha1/html/js/underscore-min.js > ... > > Obfuscated (=compiled) JavaScript looks like a binary for me: > > > When you encounter prebuilt binaries in a package you MUST: > > > > Remove all pre-built program binaries and program libraries in %prep prior to the building of the package. > > Ask upstream to remove the binaries in their next release. [5] > > [5] > https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre- > built_binaries_or_libraries > > I'm not so experienced with Fedora packaging yet, but some other distros > (you know) blame for all these *.min.js VERY strongly, so upstream might > have problems with other packages as well. Please correct me if I'm wrong. I will look into this > > -------------- > > %dir %{_sysconfdir}/%{name}/ > > It would be nice to include an example of configuration file. > Ok, will do that > -------------- > > Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in > dnsdist-debuginfo > > -------------- > > > -I/builddir/build/BUILD/dnsdist-1.0.0-alpha1/ext/mbedtls/include > > mbedtls.i686 : Light-weight cryptographic and SSL/TLS library > mbedtls.x86_64 : Light-weight cryptographic and SSL/TLS library > mbedtls-utils.x86_64 : Utilities for mbedtls > mbedtls-static.i686 : Static files for mbedtls > mbedtls-static.x86_64 : Static files for mbedtls > mbedtls-devel.i686 : Development files for mbedtls > mbedtls-devel.x86_64 : Development files for mbedtls > mbedtls-doc.noarch : Documentation files for mbedtls > ok, will look into unbundling > -------------- > > ^I--disable-silent-rules \$ > --enable-dnscrypt \$ > --enable-libsodium \$ > ^I--with-lua \$ > > (`:set list` in vim) Good catch > -------------- > > > Summary: A highly DNS-, DoS- and abuse-aware loadbalancer > > Remove the "A" article from Summary, it looks better in listings without. > It is not mandatory and is not mentioned in the guidelines, but I was > noticed about that couple times by Zbigniew Jędrzejewski-Szmek. > ok > -------------- > > Please try to use koji to ensure that package compiles on all supported > architectures: > koji build --scratch rawhide pdns-4.0.0-0.1.alpha1.fc24.src.rpm > will do > -------------- > > What the difference between https://github.com/PowerDNS/pdns and this > software? > Shouldn't dnsdist to be added as a subpackage of existing pdns package? > dnsdist lives in the pdns tree, but it is a fully seperate thing. https://www.powerdns.com/nluug/2015%20nluug%20powerdns%20dnsdist.pdf "Open source, vendor neutral - it is not “PowerDNS Dist”" > -------------- > > It seems that pdns packages also uses yahttp. > Probably yahttp needs their own package. Will look into this > > -------------- > > License and unclear relation with existing pdns package are major problems > here. Hope I have resolved these, will try to have a new spec ready soon.
Just as an update: I am working with upstream to fix some of the issues.
> Just as an update: I am working with upstream to fix some of the issues. OK.
Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec SRPM URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.2.alpha1.fc23.src.rpm This is actually not alpha1 but closer to what alpha2 will be, so at the moment you cannot verify source, but this is more to show my progress. Once alpha2 gets released I will generate a new src.rpm
Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec SRPM URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.3.alpha2.fc23.src.rpm This should resolve the issues in comment 1
(In reply to Sander Hoentjen from comment #6) > Spec URL: https://fedorapeople.org/~tjikkun/dnsdist/dnsdist.spec > SRPM URL: > https://fedorapeople.org/~tjikkun/dnsdist/dnsdist-1.0.0-0.3.alpha2.fc23.src. > rpm > > This should resolve the issues in comment 1 ACK. I'll try to review the new version on this weekend.
+ Fixed: license file is present, %license macro is used + Fixed: BuildRequires: systemd instead of systemd-utils + Fixed: %post %preun %postun scriptlets for systemd were added + Fixed: man pages installed uncompressed + Fixed: RPM spec now compiles JS from sources during %build stage + Fixed: bundled mbedtls is not used anymore + Fixed: pdns and dnsdist relation is now clear for me + license is acceptable (GPLv2) + BuildRequires are OK + %check is present and all tests pass + mock builds [1] are ok + installs and works properly [2] + rpmlint is OK - only false-positive spelling-error Package is APPROVED. Further things to consider (up to you): * it would be nice to include a sample configuration file * bundled yahttp is used at least by pdns and your package, probably yahttp needs its own package [1] tested on fedora-rawhide-x86_64 and fedora-23-armv7hl [2] tested on fedora23-armv7hl with a configuration file from https://github.com/PowerDNS/pdns/blob/master/pdns/dnsdistconf.lua
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/dnsdist
This bug was accidentally moved from POST to MODIFIED via an error in automation, please see mmccune with any questions