Koji builds;

f26 - https://koji.fedoraproject.org/koji/taskinfo?taskID=22733286
f27 - https://koji.fedoraproject.org/koji/taskinfo?taskID=22733363
epel7 - https://koji.fedoraproject.org/koji/taskinfo?taskID=22733372

Thanks for starting this Madison :) Some quick drive-by comments:

So you might want to run 'fedora-review -n kronosnet' in the folder where you have your .spec + .src.rpm kronosnet files. That will spit out a bunch of nits that we need to fix. For example:
WARNING: Cannot download url: https://github.com/fabbione/kronosnet/archive/kronosnet-0.8.tar.gz
WARNING: Package kronosnet-debuginfo-0.8-1.fc28 not built

Once we fix the URLs:
--- a/kronosnet.spec
+++ b/kronosnet.spec
@@ -72,8 +72,8 @@ Version: 0.8
 Release: 1%{?numcomm:.%{numcomm}}%{?alphatag:.%{alphatag}}%{?dirty:.%{dirty}}%{?dist}
 License: GPLv2+ and LGPLv2+
 Group: System Environment/Base
-URL: https://github.com/fabbione/kronosnet/
-Source0: https://github.com/fabbione/kronosnet/archive/%{name}-%{version}%{?numcomm:.%{numcomm}}%{?alphatag:-%{alphatag}}%{?dirty:-%{dirty}}.tar.gz
+URL: https://github.com/kronosnet/kronosnet/
+Source0: https://github.com/kronosnet/kronosnet/archive/v%{version}%{?numcomm:.%{numcomm}}%{?alphatag:-%{alphatag}}%{?dirty:-%{dirty}}.tar.gz


We will need to call autogen.sh explicitely because ./configure is not shipped in the tarball. Also, I am not sure those if defined macros are working as expected, so we'll need to look into that as well.

Please use http://www.kronosnet.org/releases/ to download release tarballs.

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=22835875

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=22835903

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=22835924

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.0.8-3
https://www.alteeve.com/an-repo/files/packages/kronosnet-0.8-3.fc26.src.rpm

I tried to use 'fedora-review', but it prompted me for a password and it wouldn't take the passwords I normally use and my google-fu was too weak to sort it out. I'll try to catch you on IRC, but in the meantime, I addressed (hopefully) your comments.

Hi, just took a quick glance.

>Group: System Environment/Base
>Group: System Environment/Libraries
>Group: Development/Libraries
The "Group" tag should not be used.

>BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
The same for "BuildRoot:".

>%install
>rm -rf %{buildroot}
You shouldn't manually remove the buildroot during %install.
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

>%doc COPYING.* COPYRIGHT 
These should probably be marked as %license.

As for fedora-review: you can try "fedora-review -rn PATH_TO_LOCAL_SRPM".

Updated to address your comments (and thanks for the package link).

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=23123556

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=23123615

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=23123672

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.0.8-4
https://www.alteeve.com/an-repo/files/packages/kronosnet-0.8-4.fc26.src.rpm

Note that I have not updated the source in a bit. I've wanted to focus on getting the spec right. Once OK, I'll build again with the latest from git.

Latest RPMs, using this morning's master from git;

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=23656614

f27: 
https://koji.fedoraproject.org/koji/taskinfo?taskID=23656681

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=23656760

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=23656730

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.0.8-5
https://www.alteeve.com/an-repo/files/packages/kronosnet-0.8-5.fc26.src.rpm

Spec URL: http://acksyn.org/files/rpms/kronosnet/kronosnet.spec
SRPM URL: http://acksyn.org/files/rpms/kronosnet/kronosnet-1.0-1.fc27.src.rpm

This is a rebuild using bandini's .spec and the SRPM source from master.

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24381668

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24381821

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24381926

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24382047

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-2
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-2.fc26.src.rpm

I'll need a sponsor to take the next step in packaging this RPM for fedora. Would anyone on this BZ be interested/able to do that for me? If not, I'll post in the main list.

Cheers

I'll happily assign myself as a reviewer if there're no complaints.

For a start, the spec is needlessly overcomplicated around conditionals:

- instead of 

> %bcond_without sctp
> [...]
> %if %{with sctp}
> %global buildsctp 1
> %endif
> [...]
> %if %{defined buildsctp}
> BuildRequires: lksctp-tools-devel
> %endif
> 
> [...]
> 
> %build
> [...]
> %{configure} \
> %if %{defined buildsctp}
>         --enable-libknet-sctp \
> %else
>         --disable-libknet-sctp \
> %endif

  please use:

> %bcond_without sctp
> [...]
> %if %{with sctp}
> BuildRequires: lksctp-tools-devel
> %endif
> 
> [...]
> 
> %build
> [...]
> %{configure} \
>   %{?with_sctp:--enable-libknet-sctp} \
>   %{!?with_sctp:--disable-libknet-sctp} \

  where the latter part can even be compacted as:

> %build
> [...]
> %{configure} \
>   --%{?with_sctp:en}%{!?with_sctp:dis}able-libknet-sctp} \

See also:
http://rpm.org/user_doc/conditional_builds.html

* * *

Another spot from this first sight, there are various pieces
I believe are already obsolete in Fedora, e.g.:

> Requires(post):   systemd-sysv
> Requires(post):   systemd-units
> Requires(preun):  systemd-units
> Requires(postun): systemd-units

See e.g.:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/LLG4T53FW2BGVZLGLKNYTKPD5SQNBZ2Y/

Thanks!!

I'll work on a new RPM shortly.

(In reply to Jan Pokorný from comment #11)
> I'll happily assign myself as a reviewer if there're no complaints.
> 
> For a start, the spec is needlessly overcomplicated around conditionals:

can you please propose a patch upstream too? I am very rusty on spec files and stuff. Digimer probably used mine as template and there is some goodness in having  them aligned. Just please make sure it builds also on opensuse.

re [comment 13]:

Mind that this is in the context of Fedora, OpenSUSE can have
conflicting guidelines and that's generally out of scope here.

(In reply to Jan Pokorný from comment #14)
> re [comment 13]:
> 
> Mind that this is in the context of Fedora, OpenSUSE can have
> conflicting guidelines and that's generally out of scope here.

A lot of the basic rpm functionalities are the same. While the fedora spec file needs/must match fedora guideline, it doesn´t prevent using some of those recommendations upstream and have them converged to be more sane on both side :-)

> %build
> [...]
> %{configure} \
>   --%{?with_sctp:en}%{!?with_sctp:dis}able-libknet-sctp} \

I think that might be the ugliest thing I've seen this year

re [comment 16]:

Hehe, with 10+ configure options, it actually becomes quite reasonable.

I am super new to this, so I am somewhat reluctant to stick my neck out, but then that's never stopped me before... :)

Jan's way is more compact, but it took me longer to parse what the string was doing. Once I got the logic, it is cleaner and smaller.

On the other hand, the original way is more verbose, but it is a bit easier to grasp (particularly for those like me who are still newish to RPM/spec). I've always *personally* been a fan of longer-if-easier-to-grasp. (This is a common debate in the perl community, where I've landed on the "no one liners!" side).

For spec though, I'm on the fence because I just don't know enough.

> re [comment 16]:
>
> Hehe, with 10+ configure options, it actually becomes quite reasonable.

No, it becomes MUCH worse. Lines are not a sparse commodity - we can use as many as we like to help make things readable. Please don't go about saving lines of code and sacrificing clarity like that. I'd rather scroll down a few lines than unscramble that mess.

Digimer, feel free to stick with middle-grounds, it's the best trade-off.

Though I am not getting the backlash against the compact version;
more lines means more redundancy (write more + read more = more
error-prone, also some brains are happier with more coarse-grained
copy-paste patterns instigating reasoning in a bit bigger picture),
and clarity is not lost at all once you absorb the logic upon the
first use as mentioned by Digimer.

Regarding licenses, we have the situation with multiple licenses
involved.  What do the guidelines[1] have to say:

> The License: field refers to the licenses of the contents of the
> binary rpm.

[...]

> If a source package generates multiple binary packages, the License:
> field may differ between them if necessary. This implies that a
> single spec may have multiple per-subpackage License: tags. 

README.license file makes the distinction clear: applications
(executables) vs. libraries.

Brief look suggest this pairing of subpackages:

* libraries:
  - lib*.rpm

* application/executable:
  - kronosnetd*.rpm

Respective subpackages should receive the license tag individually
per the above dichotomy ("LGPLv2+" and "GPLv2+", respectively).

[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines

We don't want kronosnetd packaging.

re [comment 22]:

Sure, the point is, either drop kronosnetd completely (from the
conditionals), or set the License tag properly also there.

I've rolled a new RPM with the comments from here worked in. A couple key changes;

I added a License to all packages based on the license I found at the top of the relevant C files. Please someone confirm if this is wrong.

I left the original;

> %{configure} \
>   %{?with_sctp:--enable-libknet-sctp} \
>   %{!?with_sctp:--disable-libknet-sctp} \

I commented out, but didn't fully delete, the systemd-{units,sysv}. I'd like comment from Fabio or Chrissie before fully deleting. I added a reference to the thread though.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-3
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-3.fc26.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24578635

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24578768

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24578816
*** This is failing. x86_64 build.log -> https://kojipkgs.fedoraproject.org//work/tasks/8817/24578817/build.log

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24578906

@Jan, when you are happy, can you do this so that I can take the next step in packaging? I got a sponsor so I am in the package group now. :D

Thanks!

====
20:37 < tibbs> The bug should be assigned to the person who did the review, and they should set the fedora-review flag to '+' when they're done with the review.
20:37 < tibbs> That ticket isn't assigned to anyone.
====

re: rawhide build failure; Might be a koji issue. Will try again when it appears to be fixed.

Upstream is fixing the rawhide issue (gcc8 v gcc7)

Looking at
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-3

1. I don't see any change about the clumsy conditionals
  (is it what was meant with "I left the original"?)

2. you are right that source files appear dual-licensed, but as
   mentioned, the License tag describes license of shipped artifacts
   (built executables, libraries, etc.) not of the source files,
   and that seems refined with README.license file making it clear
   under which terms are which artefacts expected to be distributed
   (binary RPMs are a form of distribution); I think particular
   License tags should reflect that -- perhaps best checked with
   upstream

3. it's customary to specify BuildRequires dependencies that are
   sourced by using pkg-config utility (*.pc files, here through
   PKG_CHECK_MODULES() macro in configure.ac file) as
   pkgconfig(foo) -- guidelines state it as SHOULD item:

   https://fedoraproject.org/wiki/Packaging:PkgConfigBuildRequires

   - for a start:
     libqb-devel -> pkgconfig(libqb)
     xz-devel    -> pkgconfig(liblzma)
     zlib-devel  -> pkgconfig(zlib)

   - also, this is likely the first time I've seen dependency on
     *-devel packages expressed via direct header file dependency,
     though configure script also asks for pkg-config module
     explicitly at least in some instances, hence I suggest:
     /usr/include/bzlib.h        -> pkgconfig(bzip2)
     /usr/include/lz4hc.h        -> pkgconfig(liblz4)
     /usr/include/nss3/nss.h     -> pkgconfig(nss)
     /usr/include/openssl/conf.h -> pkgconfig(openssl)

4. what's the purpose of fiddling with debug packages that has been
   added since last time?  it's likely inappropriate here

1. Yes, I left it as it was based on Chrissie's (strong) comments, and that it is easier to read on first pass.

2. Fabio confirmed that the licenses I entered are OK with him. Does this address the license concerns, or should a README.license be created for the RPM?

3. This is a little beyond me at this point (though I will read the link shortly). Should Fabio/Chrissie/Others comment on this, as it sounds like an upstream comment.

4. Certainly a comment for upstream.

1. Ah, I see, there's a little misunderstanding here, we indeed polemized
   about "--%{?with_sctp:en}%{!?with_sctp:dis}able-libknet-sctp}", but
   the disagreement did not cover

>   %{?with_sctp:--enable-libknet-sctp} \
>   %{!?with_sctp:--disable-libknet-sctp} \

   variant from [comment 11] (with the surrounding changes), which is hardly
   disputable and still better than the overcombined original

2. I am talking about README.license included in the tarball that's
   included in the SRPM (quick tip: you can use Midnight Commander to
   enter RPM files, and subsequently CONTENTS.cpio and any nested
   tarball that's present there), i.e., file of kronosnet proper:

   https://github.com/kronosnet/kronosnet/blob/master/README.licence

   my take is that it provides a definitive answer what (and only what)
   should License tag for libraries vs. application/executable packages
   contain -- see also [comment 21]; you may want to check this very
   conclusion with upstream, though

3. certainly a matter of advanced compiled code packaging fu (but no
   need to stress about this as we are here to help), though the
   SHOULD recommendation has its merit -- beside being nicer, it also
   offers flexibility in terms of what particular package will deliver
   the functionality requested like that, making the dependency
   expressed the most descriptive way at our disposal in Fedora

4. I mean, it may make reasons for test builds, but it will be
   catching eyes of anyone working on downstream packages needlessly

I've rolled a new RPM to address the rawhide / gcc8 issue. I also updated the project description.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-4
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-4.fc26.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24608847

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24608937

rawhide (now builds correctly):
https://koji.fedoraproject.org/koji/taskinfo?taskID=24608749

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24608951

Any comments from upstream on comment #30 or the latest RPMs?

@digimer:
I've quickly checked the https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-4.fc26.src.rpm and found following issues:

-  warning: bogus date in %changelog: Tue Jan 31 2018 Madison Kelly <mkelly> - 1.0-4  <-- Should be Wed

- capitalize the summary (so for example convert "libknet1 crypto plugins meta package" ->> "Libknet crypto....", libknet1 openssl support -> "Libknet openssl support", ...)

- Please try to follow https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires . We need explicit requires for plugins-all, but they should be properly versioned and some small comment why we need it would be nice to have.

- Please remove old cruft:

%clean
rm -rf %{buildroot}

%defattr(-,root,root,-) in %files

- I know kronosnetd is not going to be distributed right now, but fixing groupadd as https://fedoraproject.org/wiki/Packaging:UsersAndGroups#Dynamic_allocation may be good idea.

@digimer:
Few more comments to kronosned section.

It's now safe to expect systemd_post to exists so it's better to follow https://fedoraproject.org/wiki/Packaging:Systemd#Packaging, so for knet it means:
- Add buildrequire
- Remove 0%{?systemd_post:1}, 0%{?systemd_preun:1}, else sections.

I think I have integrated the comments, save for the last one in comment #34 for "buildrequire". I'm not sure what you are referring to, specifically. If I can get clarification on that, or missed anything, please let me know and I'll roll a new RPM asap.


New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-5
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-5.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24971062

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24971069

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24971077

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=24971089


Here is the diff in .spec from 1.0-4;

====
--- kronosnet.spec.1.0-4	2018-01-31 23:32:53.661198886 -0500
+++ kronosnet.spec.1.0-5	2018-02-12 14:20:52.122179273 -0500
@@ -70,10 +70,11 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.0
-Release: 3%{?dist}
+Release: 5%{?dist}
 License: GPLv2+ and LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
+Patch0: gcc8-fixes.patch
 
 # Build dependencies
 BuildRequires: gcc
@@ -83,16 +84,16 @@
 BuildRequires: lksctp-tools-devel
 %endif
 %if %{defined buildcryptonss}
-BuildRequires: /usr/include/nss3/nss.h /usr/include/nspr4/nspr.h
+BuildRequires: nss-devel nspr-devel
 %endif
 %if %{defined buildcryptoopenssl}
-BuildRequires: /usr/include/openssl/conf.h
+BuildRequires: openssl-devel
 %endif
 %if %{defined buildcompresszlib}
 BuildRequires: zlib-devel
 %endif
 %if %{defined buildcompresslz4}
-BuildRequires: /usr/include/lz4hc.h
+BuildRequires: lz4-devel
 %endif
 %if %{defined buildcompresslzo2}
 BuildRequires: lzo-devel
@@ -101,7 +102,7 @@
 BuildRequires: xz-devel
 %endif
 %if %{defined buildcompressbzip2}
-BuildRequires: /usr/include/bzlib.h
+BuildRequires: bzip2-devel
 %endif
 %if %{defined buildkronosnetd}
 BuildRequires: pam-devel
@@ -198,9 +199,6 @@
 # remove docs
 rm -rf %{buildroot}/usr/share/doc/kronosnet
 
-%clean
-rm -rf %{buildroot}
-
 # main empty package
 %description
 kronosnet source
@@ -210,16 +208,6 @@
 %package -n kronosnetd
 Summary: Multipoint-to-Multipoint VPN daemon
 License: GPLv2+ and LGPLv2+
-%if %{defined _unitdir}
-## Needed for systemd unit - Removed, see: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/LLG4T53FW2BGVZLGLKNYTKPD5SQNBZ2Y/
-#Requires(post):   systemd-sysv
-#Requires(post):   systemd-units
-#Requires(preun):  systemd-units
-#Requires(postun): systemd-units
-%else
-Requires(post): chkconfig
-Requires(preun): chkconfig, initscripts
-%endif
 Requires(post): shadow-utils
 Requires(preun): shadow-utils
 Requires: pam, /etc/pam.d/passwd
@@ -236,16 +224,8 @@
  or service disruption.
 
 %post -n kronosnetd
-%if %{defined _unitdir}
- %if 0%{?systemd_post:1}
-  %systemd_post kronosnetd.service
- %else
-  /bin/systemctl daemon-reload >/dev/null 2>&1 || :
- %endif
-%else
-/sbin/chkconfig --add kronosnetd
-%endif
-/usr/sbin/groupadd --force --system kronosnetadm
+%systemd_post kronosnetd.service
+getent group kronosnetadm >/dev/null || groupadd --force kronosnetadm
 
 %preun -n kronosnetd
 %if %{defined _unitdir}
@@ -265,7 +245,6 @@
 %endif
 
 %files -n kronosnetd
-%defattr(-,root,root,-)
 %license COPYING.* COPYRIGHT 
 %dir %{_sysconfdir}/kronosnet
 %dir %{_sysconfdir}/kronosnet/*
@@ -363,7 +342,7 @@
 
 %if %{defined buildcryptonss}
 %package -n libknet1-crypto-nss-plugin
-Summary: libknet1 nss support
+Summary: Libknet1 nss support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -377,7 +356,7 @@
 
 %if %{defined buildcryptoopenssl}
 %package -n libknet1-crypto-openssl-plugin
-Summary: libknet1 openssl support
+Summary: Libknet1 openssl support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -391,7 +370,7 @@
 
 %if %{defined buildcompresszlib}
 %package -n libknet1-compress-zlib-plugin
-Summary: libknet1 zlib support
+Summary: Libknet1 zlib support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -404,7 +383,7 @@
 %endif
 %if %{defined buildcompresslz4}
 %package -n libknet1-compress-lz4-plugin
-Summary: libknet1 lz4 and lz4hc support
+Summary: Libknet1 lz4 and lz4hc support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -419,7 +398,7 @@
 
 %if %{defined buildcompresslzo2}
 %package -n libknet1-compress-lzo2-plugin
-Summary: libknet1 lzo2 support
+Summary: Libknet1 lzo2 support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -433,7 +412,7 @@
 
 %if %{defined buildcompresslzma}
 %package -n libknet1-compress-lzma-plugin
-Summary: libknet1 lzma support
+Summary: Libknet1 lzma support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -447,7 +426,7 @@
 
 %if %{defined buildcompressbzip2}
 %package -n libknet1-compress-bzip2-plugin
-Summary: libknet1 bzip2 support
+Summary: Libknet1 bzip2 support
 License: GPLv2+ and LGPLv2+
 Requires: libknet1 = %{version}-%{release}
 
@@ -460,13 +439,13 @@
 %endif
 
 %package -n libknet1-crypto-plugins-all
-Summary: libknet1 crypto plugins meta package
+Summary: Libknet1 crypto plugins meta package
 License: GPLv2+ and LGPLv2+
 %if %{defined buildcryptonss}
-Requires: libknet1-crypto-nss-plugin
+Requires: libknet1-crypto-nss-plugin%{_isa} = %{version}-%{release}
 %endif
 %if %{defined buildcryptoopenssl}
-Requires: libknet1-crypto-openssl-plugin
+Requires: libknet1-crypto-openssl-plugin%{_isa} = %{version}-%{release}
 %endif
 
 %description -n libknet1-crypto-plugins-all
@@ -475,22 +454,22 @@
 %files -n libknet1-crypto-plugins-all
 
 %package -n libknet1-compress-plugins-all
-Summary: libknet1 compress plugins meta package
+Summary: Libknet1 compress plugins meta package
 License: GPLv2+ and LGPLv2+
 %if %{defined buildcompresszlib}
-Requires: libknet1-compress-zlib-plugin
+Requires: libknet1-compress-zlib-plugin%{_isa} = %{version}-%{release}
 %endif
 %if %{defined buildcompresslz4}
-Requires: libknet1-compress-lz4-plugin
+Requires: libknet1-compress-lz4-plugin%{_isa} = %{version}-%{release}
 %endif
 %if %{defined buildcompresslzo2}
-Requires: libknet1-compress-lzo2-plugin
+Requires: libknet1-compress-lzo2-plugin%{_isa} = %{version}-%{release}
 %endif
 %if %{defined buildcompresslzma}
-Requires: libknet1-compress-lzma-plugin
+Requires: libknet1-compress-lzma-plugin%{_isa} = %{version}-%{release}
 %endif
 %if %{defined buildcompressbzip2}
-Requires: libknet1-compress-bzip2-plugin
+Requires: libknet1-compress-bzip2-plugin%{_isa} = %{version}-%{release}
 %endif
 
 %description -n libknet1-compress-plugins-all
@@ -499,10 +478,10 @@
 %files -n libknet1-compress-plugins-all
 
 %package -n libknet1-plugins-all
-Summary: libknet1 plugins meta package
+Summary: Libknet1 plugins meta package
 License: GPLv2+ and LGPLv2+
-Requires: libknet1-compress-plugins-all
-Requires: libknet1-crypto-plugins-all
+Requires: libknet1-compress-plugins-all%{_isa} = %{version}-%{release}
+Requires: libknet1-crypto-plugins-all%{_isa} = %{version}-%{release}
 
 %description -n libknet1-plugins-all
  meta package to install all of libknet1 plugins
@@ -514,7 +493,15 @@
 %endif
 
 %changelog
-* Tue Jan 31 2018 Madison Kelly <mkelly> - 1.0-4
+* Mon Feb 12 2018 Madison Kelly <mkelly> - 1.0-5
+- All changes related to this spec;
+- Fixed typo in previous changelog date (Tue -> Wed).
+- Capitalized summaries as needed.
+- Changed Requires that were file paths to package names.
+- Added arch and version/release requirements for knet packages.
+- Changed groupadd to be a little smarter.
+
+* Wed Jan 31 2018 Madison Kelly <mkelly> - 1.0-4
 - Added patch for gcc8 fix.
 - Cleaned up description.
====

@digimer:
From my point of view, package is almost ready to pass all the required test, but there are still few left:

- Please remove %defattr(-,root,root,-) everywhere

- For kronosnetd:
  * Requires(preun): shadow-utils <-- not needed

  * groupadd --force kronosnetadm <-- kronosnetadm should be system account (and force is not needed), so result should be "groupadd -r kronosnetadm"

  * %systemd_post kronosnetd.service <-- I believe there was small misunderstanding. So the question is, if you want to keep compatibility with non-systemd systems (whole spec is written that way), then stanza should look like:

    ```
    %if %{defined _unitdir}
      %systemd_post kronosnetd.service
    %else
    /sbin/chkconfig --add kronosnetd
    %endif
    ```
  * Please fix the "%preun -n kronosnetd" section similar way

  * Add %{?systemd_requires} (conditionally)

  * Add postun:
    ```
    %postun -n kronosnetd
    %if %{defined _unitdir}
      %systemd_postun kronosnetd.service
    %endif
```

- Missing "BuildRequires: systemd" (probably conditional) so something like
  ```
  %if %{defined _unitdir} && %{defined buildkronosnetd}
    BuildRequires: systemd
  %endif
  ```

@digimer:
Forgot one more thing. We need more %{_isa}. Just add them basically for every

Requires: libtap1

and

Requires: libknet1

> - Please remove %defattr(-,root,root,-) everywhere

Done.

> - For kronosnetd:
>  * Requires(preun): shadow-utils <-- not needed

What about the (postrun) entry? I've left that for now.

>   * groupadd --force kronosnetadm <-- kronosnetadm should be system account (and force is not needed), so result should be "groupadd -r kronosnetadm"

OK, but I used the more verbose '--system' just to be a little more self-documenting.

> So the question is, if you want to keep compatibility with non-systemd systems

No, I see no reason for this. Knet will never run on EL6. I do test against EPEL7 and modern Fedoras, which are all systemd now. I'd actually prefer to remove all sysvinit stuff down the road. I'm only not doing it now because I still have too much to learn and I didn't want to slow down the initial release.

Now that it is an issue though (if minor), I will try to remove it in this release. I apologize if this introduces new issues at this stage.

> - Missing "BuildRequires: systemd" (probably conditional) so something like

Given the above comment, I added it without conditional. 

> Forgot one more thing. We need more %{_isa}. Just add them basically for every...

Done.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-6
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-6.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25061282

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25061295

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25061303

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25061269


Here is the diff in .spec from 1.0-5;

====
--- kronosnet.spec.1.0-5	2018-02-12 14:20:52.122179273 -0500
+++ kronosnet.spec.1.0-6	2018-02-15 00:25:25.797016016 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.0
-Release: 5%{?dist}
+Release: 6%{?dist}
 License: GPLv2+ and LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -78,6 +78,7 @@
 
 # Build dependencies
 BuildRequires: gcc
+BuildRequires: systemd
 # required to build man pages
 BuildRequires: libqb-devel libxml2-devel doxygen
 %if %{defined buildsctp}
@@ -170,11 +171,7 @@
 	--enable-libtap \
 %endif
 	--with-initdefaultdir=%{_sysconfdir}/sysconfig/ \
-%if %{defined _unitdir}
 	--with-systemddir=%{_unitdir}
-%else
-	--with-initddir=%{_sysconfdir}/rc.d/init.d/
-%endif
 
 make %{_smp_mflags}
 
@@ -188,13 +185,8 @@
 find %{buildroot} -name "*.la" -exec rm {} \;
 
 # handle systemd vs init script
-%if %{defined _unitdir}
 # remove init scripts
 rm -rf %{buildroot}/etc/init.d
-%else
-# remove systemd specific bits
-find %{buildroot} -name "*.service" -exec rm {} \;
-%endif
 
 # remove docs
 rm -rf %{buildroot}/usr/share/doc/kronosnet
@@ -209,7 +201,6 @@
 Summary: Multipoint-to-Multipoint VPN daemon
 License: GPLv2+ and LGPLv2+
 Requires(post): shadow-utils
-Requires(preun): shadow-utils
 Requires: pam, /etc/pam.d/passwd
 
 %description -n kronosnetd
@@ -228,21 +219,14 @@
 getent group kronosnetadm >/dev/null || groupadd --force kronosnetadm
 
 %preun -n kronosnetd
-%if %{defined _unitdir}
- %if 0%{?systemd_preun:1}
+%if 0%{?systemd_preun:1}
   %systemd_preun kronosnetd.service
- %else
+%else
 if [ "$1" -eq 0 ]; then
 	/bin/systemctl --no-reload disable kronosnetd.service
 	/bin/systemctl stop kronosnetd.service >/dev/null 2>&1
 fi
 %endif
-%else
-if [ "$1" = 0 ]; then
-	/sbin/service kronosnetd stop >/dev/null 2>&1
-	/sbin/chkconfig --del kronosnetd
-fi
-%endif
 
 %files -n kronosnetd
 %license COPYING.* COPYRIGHT 
@@ -251,11 +235,7 @@
 %config(noreplace) %{_sysconfdir}/sysconfig/kronosnetd
 %config(noreplace) %{_sysconfdir}/pam.d/kronosnetd
 %config(noreplace) %{_sysconfdir}/logrotate.d/kronosnetd
-%if %{defined _unitdir}
 %{_unitdir}/kronosnetd.service
-%else
-%config(noreplace) %{_sysconfdir}/rc.d/init.d/kronosnetd
-%endif
 %{_sbindir}/*
 %{_mandir}/man8/*
 %endif
@@ -271,7 +251,6 @@
  pre-up.d/up.d/down.d/post-down.d infrastructure.
 
 %files -n libtap1
-%defattr(-,root,root,-)
 %license COPYING.* COPYRIGHT
 %{_libdir}/libtap.so.*
 
@@ -282,7 +261,7 @@
 %package -n libtap1-devel
 Summary: Simple userland wrapper around kernel tap devices (developer files)
 License: GPLv2+ and LGPLv2+
-Requires: libtap1 = %{version}-%{release}
+Requires: libtap1%{_isa} = %{version}-%{release}
 Requires: pkgconfig
 
 %description -n libtap1-devel
@@ -291,7 +270,6 @@
  pre-up.d/up.d/down.d/post-down.d infrastructure.
 
 %files -n libtap1-devel
-%defattr(-,root,root,-)
 %license COPYING.* COPYRIGHT
 %{_libdir}/libtap.so
 %{_includedir}/libtap.h
@@ -312,7 +290,6 @@
  Please refer to https://kronosnet.org/ for further  information.
 
 %files -n libknet1
-%defattr(-,root,root,-)
 %license COPYING.* COPYRIGHT
 %{_libdir}/libknet.so.*
 %dir %{_libdir}/kronosnet
@@ -324,7 +301,7 @@
 %package -n libknet1-devel
 Summary: Kronosnet core switching implementation (developer files)
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 Requires: pkgconfig
 
 %description -n libknet1-devel
@@ -333,7 +310,6 @@
  information. 
 
 %files -n libknet1-devel
-%defattr(-,root,root,-)
 %license COPYING.* COPYRIGHT
 %{_libdir}/libknet.so
 %{_includedir}/libknet.h
@@ -344,13 +320,12 @@
 %package -n libknet1-crypto-nss-plugin
 Summary: Libknet1 nss support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-crypto-nss-plugin
  NSS crypto support for libknet1.
 
 %files -n libknet1-crypto-nss-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/crypto_nss.so
 %endif
 
@@ -358,13 +333,12 @@
 %package -n libknet1-crypto-openssl-plugin
 Summary: Libknet1 openssl support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-crypto-openssl-plugin
  OpenSSL crypto support for libknet1.
 
 %files -n libknet1-crypto-openssl-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/crypto_openssl.so
 %endif
 
@@ -372,26 +346,24 @@
 %package -n libknet1-compress-zlib-plugin
 Summary: Libknet1 zlib support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-zlib-plugin
  zlib compression support for libknet1.
 
 %files -n libknet1-compress-zlib-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/compress_zlib.so
 %endif
 %if %{defined buildcompresslz4}
 %package -n libknet1-compress-lz4-plugin
 Summary: Libknet1 lz4 and lz4hc support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lz4-plugin
  lz4 and lz4hc compression support for libknet1.
 
 %files -n libknet1-compress-lz4-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/compress_lz4.so
 %{_libdir}/kronosnet/compress_lz4hc.so
 %endif
@@ -400,13 +372,12 @@
 %package -n libknet1-compress-lzo2-plugin
 Summary: Libknet1 lzo2 support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lzo2-plugin
  lzo2 compression support for libknet1.
 
 %files -n libknet1-compress-lzo2-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/compress_lzo2.so
 %endif
 
@@ -414,13 +385,12 @@
 %package -n libknet1-compress-lzma-plugin
 Summary: Libknet1 lzma support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lzma-plugin
  lzma compression support for libknet1.
 
 %files -n libknet1-compress-lzma-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/compress_lzma.so
 %endif
 
@@ -428,13 +398,12 @@
 %package -n libknet1-compress-bzip2-plugin
 Summary: Libknet1 bzip2 support
 License: GPLv2+ and LGPLv2+
-Requires: libknet1 = %{version}-%{release}
+Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-bzip2-plugin
  bzip2 compression support for libknet1.
 
 %files -n libknet1-compress-bzip2-plugin
-%defattr(-,root,root,-)
 %{_libdir}/kronosnet/compress_bzip2.so
 %endif
====

@digimer:
- You've forgot to update changelog

- For kronosnetd:
  * As was noted in previous comment, please add postun
    ```
    %postun -n kronosnetd
    %systemd_postun kronosnetd.service
    ```
  * Please fix the "%preun -n kronosnetd" section similar way as postun (= no need to check 0%{?systemd_preun:1})
  * Add %{?systemd_requires} as described in https://fedoraproject.org/wiki/Packaging:Scriptlets#Systemd

That's all from me.

Some of the @poki comments are still not solved and may be useful:
- License - It took me a while to parse what poki meant, and I can agree with him that License field may be more specific. So main idea is:
  * License of kronosnetd should be GPLv2+, because it is application
  * License of libknet* should be LGPLv2+, simply because they are libraries

  But let's quickly check with Fabio what he thinks about it.

- Idea of using pkgconfig(foo) seems to be quite nice, could you please give it a try? (just to recap):
     libqb-devel -> pkgconfig(libqb)
     xz-devel    -> pkgconfig(liblzma)
     zlib-devel  -> pkgconfig(zlib)
     bzip2-devel -> pkgconfig(bzip2)
     lz4-devel -> pkgconfig(liblz4)
     nss-devel -> pkgconfig(nss) # We don't need to require on nspr-devel then
     openssl-devel -> pkgconfig(openssl)

> You've forgot to update changelog

Doh! Added (for 1.0-6 and -7)

> As was noted in previous comment, please add postun

Added, but please verify I added in the correct location.

> Please fix the "%preun -n kronosnetd" section...

Being an if/else, with 'if [ "$1" -eq 0 ]; ...' being in the 'else', I removed that as well. Was that correct?

> Add %{?systemd_requires} as described in ...

Done. 

> License - ...

Done.

> Idea of using pkgconfig(foo) seems to be quite nice, could you please give it a try?

Done. Question though; I wasn't able to find a clear explanation about what this change does (see: https://fedoraproject.org/wiki/Packaging:Guidelines#Pkgconfig_Files_.28foo.pc.29). Why would we not also wrap, say, libxml2-devel, lksctp-tools-devel, etc in a similar manner?

NOTE: It would seem that one of these changes breaks epel7 build. I will dig into this and post a new .spec if I can sort it out. For now, this one is up for you to review as it currently stands.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-7
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-7.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25080933

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25080940

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25080948

epel7: *Failed*
https://koji.fedoraproject.org/koji/taskinfo?taskID=25080956

Here is the diff in .spec from 1.0-5;

====
--- kronosnet.spec.1.0-6	2018-02-15 00:25:25.797016016 -0500
+++ kronosnet.spec.1.0-7	2018-02-15 19:18:05.854232031 -0500
@@ -70,40 +70,42 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.0
-Release: 6%{?dist}
-License: GPLv2+ and LGPLv2+
+Release: 7%{?dist}
+License: GPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
 Patch0: gcc8-fixes.patch
 
 # Build dependencies
 BuildRequires: gcc
+%{?systemd_requires}
 BuildRequires: systemd
 # required to build man pages
-BuildRequires: libqb-devel libxml2-devel doxygen
+BuildRequires: libxml2-devel doxygen
+BuildRequires: pkgconfig(libqb)
 %if %{defined buildsctp}
 BuildRequires: lksctp-tools-devel
 %endif
 %if %{defined buildcryptonss}
-BuildRequires: nss-devel nspr-devel
+BuildRequires: pkgconfig(nss)
 %endif
 %if %{defined buildcryptoopenssl}
-BuildRequires: openssl-devel
+BuildRequires: pkgconfig(openssl)
 %endif
 %if %{defined buildcompresszlib}
-BuildRequires: zlib-devel
+BuildRequires: pkgconfig(zlib)
 %endif
 %if %{defined buildcompresslz4}
-BuildRequires: lz4-devel
+BuildRequires: pkgconfig(liblz4)
 %endif
 %if %{defined buildcompresslzo2}
 BuildRequires: lzo-devel
 %endif
 %if %{defined buildcompresslzma}
-BuildRequires: xz-devel
+BuildRequires: pkgconfig(liblzma)
 %endif
 %if %{defined buildcompressbzip2}
-BuildRequires: bzip2-devel
+BuildRequires: pkgconfig(bzip2)
 %endif
 %if %{defined buildkronosnetd}
 BuildRequires: pam-devel
@@ -199,7 +201,7 @@
 ## Runtime and subpackages section
 %package -n kronosnetd
 Summary: Multipoint-to-Multipoint VPN daemon
-License: GPLv2+ and LGPLv2+
+License: GPLv2+
 Requires(post): shadow-utils
 Requires: pam, /etc/pam.d/passwd
 
@@ -218,15 +220,11 @@
 %systemd_post kronosnetd.service
 getent group kronosnetadm >/dev/null || groupadd --force kronosnetadm
 
+%postun -n kronosnetd
+%systemd_postun kronosnetd.service
+
 %preun -n kronosnetd
-%if 0%{?systemd_preun:1}
-  %systemd_preun kronosnetd.service
-%else
-if [ "$1" -eq 0 ]; then
-	/bin/systemctl --no-reload disable kronosnetd.service
-	/bin/systemctl stop kronosnetd.service >/dev/null 2>&1
-fi
-%endif
+%systemd_preun kronosnetd.service
 
 %files -n kronosnetd
 %license COPYING.* COPYRIGHT 
@@ -243,7 +241,7 @@
 %if %{defined buildlibtap}
 %package -n libtap1
 Summary: Simple userland wrapper around kernel tap devices
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 
 %description -n libtap1
  This is an over-engineered commodity library to manage a pool
@@ -260,7 +258,7 @@
 
 %package -n libtap1-devel
 Summary: Simple userland wrapper around kernel tap devices (developer files)
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libtap1%{_isa} = %{version}-%{release}
 Requires: pkgconfig
 
@@ -278,7 +276,7 @@
 
 %package -n libknet1
 Summary: Kronosnet core switching implementation
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 
 %description -n libknet1
  Kronosnet, often referred to as knet, is a network abstraction layer 
@@ -300,7 +298,7 @@
 
 %package -n libknet1-devel
 Summary: Kronosnet core switching implementation (developer files)
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 Requires: pkgconfig
 
@@ -319,7 +317,7 @@
 %if %{defined buildcryptonss}
 %package -n libknet1-crypto-nss-plugin
 Summary: Libknet1 nss support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-crypto-nss-plugin
@@ -332,7 +330,7 @@
 %if %{defined buildcryptoopenssl}
 %package -n libknet1-crypto-openssl-plugin
 Summary: Libknet1 openssl support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-crypto-openssl-plugin
@@ -345,7 +343,7 @@
 %if %{defined buildcompresszlib}
 %package -n libknet1-compress-zlib-plugin
 Summary: Libknet1 zlib support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-zlib-plugin
@@ -357,7 +355,7 @@
 %if %{defined buildcompresslz4}
 %package -n libknet1-compress-lz4-plugin
 Summary: Libknet1 lz4 and lz4hc support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lz4-plugin
@@ -371,7 +369,7 @@
 %if %{defined buildcompresslzo2}
 %package -n libknet1-compress-lzo2-plugin
 Summary: Libknet1 lzo2 support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lzo2-plugin
@@ -384,7 +382,7 @@
 %if %{defined buildcompresslzma}
 %package -n libknet1-compress-lzma-plugin
 Summary: Libknet1 lzma support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lzma-plugin
@@ -397,7 +395,7 @@
 %if %{defined buildcompressbzip2}
 %package -n libknet1-compress-bzip2-plugin
 Summary: Libknet1 bzip2 support
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-bzip2-plugin
@@ -409,7 +407,7 @@
 
 %package -n libknet1-crypto-plugins-all
 Summary: Libknet1 crypto plugins meta package
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 %if %{defined buildcryptonss}
 Requires: libknet1-crypto-nss-plugin%{_isa} = %{version}-%{release}
 %endif
@@ -424,7 +422,7 @@
 
 %package -n libknet1-compress-plugins-all
 Summary: Libknet1 compress plugins meta package
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 %if %{defined buildcompresszlib}
 Requires: libknet1-compress-zlib-plugin%{_isa} = %{version}-%{release}
 %endif
@@ -448,7 +446,7 @@
 
 %package -n libknet1-plugins-all
 Summary: Libknet1 plugins meta package
-License: GPLv2+ and LGPLv2+
+License: LGPLv2+
 Requires: libknet1-compress-plugins-all%{_isa} = %{version}-%{release}
 Requires: libknet1-crypto-plugins-all%{_isa} = %{version}-%{release}
 
@@ -462,6 +460,19 @@
 %endif
 
 %changelog
+* Thu Feb 15 2018 Madison Kelly <mkelly> - 1.0-7
+- Added missing 1.0-6 changelog.
+- Added kronosnetd postun.
+- Clarified licensing.
+- (re)added systemd_requires.
+- Wrapped several buildrequires with pkgconfig().
+
+* Wed Feb 14 2018 Madison Kelly <mkelly> - 1.0-6
+- Removed sysvinit checks.
+- Fixed the groupadd to add to system group.
+- Added build requirement for systemd.
+- Added %{_isa} to knet requirements in sub packages.
+
 * Mon Feb 12 2018 Madison Kelly <mkelly> - 1.0-5
 - All changes related to this spec;
 - Fixed typo in previous changelog date (Tue -> Wed).
====

This is the change that breaks EPEL7;

-BuildRequires: bzip2-devel
+BuildRequires: pkgconfig(bzip2)

If I roll that back, it builds fine on RHEL 7. With the 'pkgconfig(bzip2)', I get this:

====
[root@el7-builder-t1 ~]# yum install bzip2-devel
Loaded plugins: kabi, product-id, search-disabled-repos, subscription-manager
Loading support for Red Hat kernel ABI
Package bzip2-devel-1.0.6-13.el7.x86_64 already installed and latest version
Nothing to do

[root@el7-builder-t1 ~]# su - digimer
Last login: Thu Feb 15 19:30:30 EST 2018 on pts/0
[digimer@el7-builder-t1 ~]$ cd rpmbuild/SPECS/

[digimer@el7-builder-t1 SPECS]$ rpmbuild -ba kronosnet.spec 
error: Failed build dependencies:
	pkgconfig(bzip2) is needed by kronosnet-1.0-7.el7.x86_64
====

Any insight? Or shall I just revert that one BuildRequires?

Speaking to Fabio, and given the way the comment around 'pkgconfig()' was framed, I rolled back the BuildRequires for bzip2.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-8
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-8.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25084124

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25084134

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25084142

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25084066


Diff from 1.0-7:
====
--- kronosnet.spec.1.0-7	2018-02-15 19:18:05.854232031 -0500
+++ kronosnet.spec.1.0-8	2018-02-16 00:04:45.452029189 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.0
-Release: 7%{?dist}
+Release: 8%{?dist}
 License: GPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -105,7 +105,7 @@
 BuildRequires: pkgconfig(liblzma)
 %endif
 %if %{defined buildcompressbzip2}
-BuildRequires: pkgconfig(bzip2)
+BuildRequires: bzip2-devel
 %endif
 %if %{defined buildkronosnetd}
 BuildRequires: pam-devel
@@ -460,6 +460,9 @@
 %endif
 
 %changelog
+* Fri Feb 16 2018 Madison Kelly <mkelly> - 1.0-8
+- Reverted to 'BuildRequires: bzip2-devel' to fix EPEL7 builds.
+
 * Thu Feb 15 2018 Madison Kelly <mkelly> - 1.0-7
 - Added missing 1.0-6 changelog.
 - Added kronosnetd postun.
====

@digimer:
- Postun is on correct location, no worries

- "%preun -n kronosnetd" is also corrent

- But %{?systemd_requires} is not on correct location. It basically expands to:
  ```
  Requires(post): systemd
  Requires(preun): systemd
  Requires(postun): systemd
  ```
  so it should be in the kronosnetd part. (do not move BuildRequires: systemd, just %{?systemd_requires} ).

- About license, I would let kronosnet License (only main package, other should be as they are) field as it was (so GPL + LGPL), because this is correct license for SRPM.

- pkgconfig was (at least from my side) optional so nice job.

Please fix the two things (License + location of %{?systemd_requires}) and knet should be good to go.

@digimer:
Also please use official release from http://www.kronosnet.org/releases in srpm, not something else (as is the case now). Checksum then differs.

Sorry for the delay.

Changes made (tarball from release, license and systed_requires). 

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-9
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-9.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25239823

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25239830

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25239815

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25239839


Diff from 1.0-8:
====
--- kronosnet.spec.1.0-8	2018-02-16 00:04:45.452029189 -0500
+++ kronosnet.spec.1.0-9	2018-02-22 14:34:54.588904329 -0500
@@ -70,15 +70,14 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.0
-Release: 8%{?dist}
-License: GPLv2+
+Release: 9%{?dist}
+License: GPLv2+ + LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
 Patch0: gcc8-fixes.patch
 
 # Build dependencies
 BuildRequires: gcc
-%{?systemd_requires}
 BuildRequires: systemd
 # required to build man pages
 BuildRequires: libxml2-devel doxygen
@@ -204,6 +203,7 @@
 License: GPLv2+
 Requires(post): shadow-utils
 Requires: pam, /etc/pam.d/passwd
+%{?systemd_requires}
 
 %description -n kronosnetd
 The kronosnet daemon is a bridge between kronosnet switching engine
====

@digimer:
Perfect. Spec file now looks really great with one small nitpick - changelog is not updated. Please keep that in mind, I believe you also find useful when package has good changelog.

That's the second time I missed that... >_<.

I'll roll a final RPM today/tonight with the change log updated.

Madi

Updated to add the missing change log.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.0-10
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.0-10.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25264181

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25264214

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25264225

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25264234


Diff from 1.0-9:
====
--- kronosnet.spec.1.0-9	2018-02-22 14:34:54.588904329 -0500
+++ kronosnet.spec.1.0-10	2018-02-23 14:47:17.586627823 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.0
-Release: 9%{?dist}
+Release: 10%{?dist}
 License: GPLv2+ + LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -460,6 +460,14 @@
 %endif
 
 %changelog
+* Fri Feb 23 2018 Madison Kelly <mkelly> - 1.0-10
+- Added missing change log for 1.0-9.
+
+* Thu Feb 22 2018 Madison Kelly <mkelly> - 1.0-9
+- Changed the source tarball to be one from the upstream source.
+- Updated the main license.
+- Moved down %{?systemd_requires}.
+
 * Fri Feb 16 2018 Madison Kelly <mkelly> - 1.0-8
 - Reverted to 'BuildRequires: bzip2-devel' to fix EPEL7 builds.
====

Rerolled for the new 1.1 release.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-1
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-1.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25302495

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25302502

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25302512

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25302490


Diff from 1.0-10:
====
--- kronosnet.spec.1.0-10	2018-02-23 14:47:17.586627823 -0500
+++ kronosnet.spec.1.1-1	2018-02-25 13:17:20.497971906 -0500
@@ -69,12 +69,11 @@
 
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
-Version: 1.0
-Release: 10%{?dist}
+Version: 1.1
+Release: 1%{?dist}
 License: GPLv2+ + LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
-Patch0: gcc8-fixes.patch
 
 # Build dependencies
 BuildRequires: gcc
@@ -117,7 +116,6 @@
 
 %prep
 %setup -q -n %{name}-%{version}
-%patch0 -p1 -z gcc8-fixes.patch
 
 %build
 %if %{with runautogen}
@@ -277,6 +275,8 @@
 %package -n libknet1
 Summary: Kronosnet core switching implementation
 License: LGPLv2+
+BuildRequires: libqb-devel
+BuildRequires: doxygen
 
 %description -n libknet1
  Kronosnet, often referred to as knet, is a network abstraction layer 
@@ -460,6 +460,11 @@
 %endif
 
 %changelog
+* Sun Feb 25 2018 Madison Kelly <mkelly> - 1.1-1
+- Rerolled for 1.1 upstream release.
+- Removed the (no longer needed) gcc8-fixes.patch
+- Added the new doxygen and libqb-devel buildrequires for libknetd.
+
 * Fri Feb 23 2018 Madison Kelly <mkelly> - 1.0-10
 - Added missing change log for 1.0-9.
====

Slight change as per upstream comment to make 'systemd' build dep in the kronosnetd conditional.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-2
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-2.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25311003

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25310992

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25310980

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25311010


Diff from 1.1-1:
====
--- kronosnet.spec.1.1-1	2018-02-25 13:17:20.497971906 -0500
+++ kronosnet.spec.1.1-2	2018-02-25 23:17:33.268040008 -0500
@@ -70,14 +70,13 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.1
-Release: 1%{?dist}
+Release: 2%{?dist}
 License: GPLv2+ + LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
 
 # Build dependencies
 BuildRequires: gcc
-BuildRequires: systemd
 # required to build man pages
 BuildRequires: libxml2-devel doxygen
 BuildRequires: pkgconfig(libqb)
@@ -106,6 +105,7 @@
 BuildRequires: bzip2-devel
 %endif
 %if %{defined buildkronosnetd}
+BuildRequires: systemd
 BuildRequires: pam-devel
 %endif
 %if %{defined buildautogen}
@@ -460,6 +460,9 @@
 %endif
 
 %changelog
+* Sun Feb 25 2018 Madison Kelly <mkelly> - 1.1-2
+- Moved the 'BuildRequires: systemd' to be conditional with kronostnetd.
+
 * Sun Feb 25 2018 Madison Kelly <mkelly> - 1.1-1
 - Rerolled for 1.1 upstream release.
 - Removed the (no longer needed) gcc8-fixes.patch
====

@digimer:
Perfect, looks really nice, and good work with packaging latest version.

I've just noticed one small problem - again with changelog - and it's using of macros in the changelog. This is problem, because macros are expanded no matter that they are in the changelog section. So please remove %{_isa} and %{?systemd_requires} otherwise changelog gets screwed.

Fixed.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-3
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-3.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25322696

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25322704

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25322713

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25322722


Diff from 1.1-2:
====
--- kronosnet.spec.1.1-2	2018-02-25 23:17:33.268040008 -0500
+++ kronosnet.spec.1.1-3	2018-02-26 10:03:13.304992918 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.1
-Release: 2%{?dist}
+Release: 3%{?dist}
 License: GPLv2+ + LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -460,6 +460,9 @@
 %endif
 
 %changelog
+* Mon Feb 26 2018 Madison Kelly <mkelly> - 1.1-3
+- Fixed the changelog to not have the full macro names.
+
 * Sun Feb 25 2018 Madison Kelly <mkelly> - 1.1-2
 - Moved the 'BuildRequires: systemd' to be conditional with kronostnetd.
 
@@ -474,7 +477,7 @@
 * Thu Feb 22 2018 Madison Kelly <mkelly> - 1.0-9
 - Changed the source tarball to be one from the upstream source.
 - Updated the main license.
-- Moved down %{?systemd_requires}.
+- Moved down the {?systemd_requires} macro.
 
 * Fri Feb 16 2018 Madison Kelly <mkelly> - 1.0-8
 - Reverted to 'BuildRequires: bzip2-devel' to fix EPEL7 builds.
@@ -490,7 +493,7 @@
 - Removed sysvinit checks.
 - Fixed the groupadd to add to system group.
 - Added build requirement for systemd.
-- Added %{_isa} to knet requirements in sub packages.
+- Added {_isa} macro to knet requirements in sub packages.
 
 * Mon Feb 12 2018 Madison Kelly <mkelly> - 1.0-5
 - All changes related to this spec;
====

Package is ok to go into Fedora.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package does not own files or directories owned by other packages.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: kronosnet-debuginfo-1.1-3.fc28.i686.rpm
          kronosnet-debugsource-1.1-3.fc28.i686.rpm
          libknet1-1.1-3.fc28.i686.rpm
          libknet1-devel-1.1-3.fc28.i686.rpm
          libknet1-crypto-nss-plugin-1.1-3.fc28.i686.rpm
          libknet1-crypto-openssl-plugin-1.1-3.fc28.i686.rpm
          libknet1-compress-zlib-plugin-1.1-3.fc28.i686.rpm
          libknet1-compress-lz4-plugin-1.1-3.fc28.i686.rpm
          libknet1-compress-lzo2-plugin-1.1-3.fc28.i686.rpm
          libknet1-compress-lzma-plugin-1.1-3.fc28.i686.rpm
          libknet1-compress-bzip2-plugin-1.1-3.fc28.i686.rpm
          libknet1-crypto-plugins-all-1.1-3.fc28.i686.rpm
          libknet1-compress-plugins-all-1.1-3.fc28.i686.rpm
          libknet1-plugins-all-1.1-3.fc28.i686.rpm
          kronosnet-1.1-3.fc28.src.rpm
kronosnet-debuginfo.i686: W: invalid-license GPLv2+ + LGPLv2+
kronosnet-debugsource.i686: W: invalid-license GPLv2+ + LGPLv2+
kronosnet-debugsource.i686: W: no-documentation
libknet1.i686: W: spelling-error Summary(en_US) Kronosnet -> Kronecker
libknet1.i686: W: spelling-error %description -l en_US Kronosnet -> Kronecker
libknet1.i686: W: spelling-error %description -l en_US knet -> net, knelt, knee
libknet1.i686: W: no-documentation
libknet1-devel.i686: W: spelling-error Summary(en_US) Kronosnet -> Kronecker
libknet1-devel.i686: W: spelling-error %description -l en_US kronosnet -> Kronecker
libknet1-crypto-nss-plugin.i686: W: no-documentation
libknet1-crypto-openssl-plugin.i686: W: no-documentation
libknet1-compress-zlib-plugin.i686: W: no-documentation
libknet1-compress-lz4-plugin.i686: W: no-documentation
libknet1-compress-lzo2-plugin.i686: W: no-documentation
libknet1-compress-lzma-plugin.i686: W: no-documentation
libknet1-compress-bzip2-plugin.i686: W: no-documentation
libknet1-crypto-plugins-all.i686: W: no-documentation
libknet1-compress-plugins-all.i686: W: no-documentation
libknet1-plugins-all.i686: W: no-documentation
kronosnet.src: W: spelling-error Summary(en_US) Multipoint -> Multipurpose
kronosnet.src: W: invalid-license GPLv2+ + LGPLv2+
15 packages and 0 specfiles checked; 0 errors, 21 warnings.




Rpmlint (debuginfo)
-------------------
Checking: kronosnet-debuginfo-1.1-3.fc28.i686.rpm
kronosnet-debuginfo.i686: W: invalid-license GPLv2+ + LGPLv2+
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Re macros in %changelog:

Common way of escaping that convey what was meant down the road
(e.g. to rpm -q --changelog query) is to use doubled '%', i.e.,
%{_isa} -> %%{_isa}, not to drop the per cent character.


Some concerns that remain:

A. [Comment 28] 4.: no reason to mangle with debuginfo generation
- one can always use command-line switches to achieve the same:
  http://lists.rpm.org/pipermail/rpm-list/2013-April/001416.html
  (definitely not a mainstream need, even less in Fedora context)

B. [Comment 11]: I'd still suggest using

%{configure} \
>   %{?with_sctp:--enable-libknet-sctp} \
>   %{!?with_sctp:--disable-libknet-sctp} \
> [...]

Reason is also practical, e.g. the whole "configure" statement
barely fits a single laptop screen for me currently, because
the notation of choice is excessively line-hungry.


Also, please:

C. Refrain from initial spaces/indentation in %description-s.

D. Check whether there are some tests that could be run as part of
   the build under %check scriptlet (to be added if that's the case).

E. Because libknet1-devel requires (indirectly) libknet1, it may
   drop the %license files, as these will be present thanks to
   libknet1 installed in parallel, hence satisfying:
   https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Subpackage_Licensing


Rather for future consideration:
- if the documentation for the API functions will keep growing,
  it might be desirable to split those manpages to a separate
  subpackage:
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
("Or if there's a lot of documentation...")

Re macros in %changelog:

Common way of escaping that convey what was meant down the road
(e.g. to rpm -q --changelog query) is to use doubled '%', i.e.,
%{_isa} -> %%{_isa}, not to drop the per cent character.


Some concerns that remain:

A. [Comment 28] 4.: no reason to mangle with debuginfo generation
- one can always use command-line switches to achieve the same:
  http://lists.rpm.org/pipermail/rpm-list/2013-April/001416.html
  (definitely not a mainstream need, even less in Fedora context)

B. [Comment 11]: I'd still suggest using

%{configure} \
>   %{?with_sctp:--enable-libknet-sctp} \
>   %{!?with_sctp:--disable-libknet-sctp} \
> [...]

Reason is also practical, e.g. the whole "configure" statement
barely fits a single laptop screen for me currently, because
the notation of choice is excessively line-hungry.


Also, please:

C. Refrain from initial spaces/indentation in %description-s.

D. Check whether there are some tests that could be run as part of
   the build under %check scriptlet (to be added if that's the case).

E. Because libknet1-devel requires (indirectly) libknet1, it may
   drop the %license files, as these will be present thanks to
   libknet1 installed in parallel, hence satisfying:
   https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Subpackage_Licensing


Rather for future consideration:
- if the documentation for the API functions will keep growing,
  it might be desirable to split those manpages to a separate
  subpackage:
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
("Or if there's a lot of documentation...")

(In reply to Jan Pokorný from comment #55)
> Re macros in %changelog:
> 
> Common way of escaping that convey what was meant down the road
> (e.g. to rpm -q --changelog query) is to use doubled '%', i.e.,
> %{_isa} -> %%{_isa}, not to drop the per cent character.
> 
> 
> Some concerns that remain:
> 
> A. [Comment 28] 4.: no reason to mangle with debuginfo generation
> - one can always use command-line switches to achieve the same:
>   http://lists.rpm.org/pipermail/rpm-list/2013-April/001416.html
>   (definitely not a mainstream need, even less in Fedora context)
> 
> B. [Comment 11]: I'd still suggest using
> 
> %{configure} \
> >   %{?with_sctp:--enable-libknet-sctp} \
> >   %{!?with_sctp:--disable-libknet-sctp} \
> > [...]
> 
> Reason is also practical, e.g. the whole "configure" statement
> barely fits a single laptop screen for me currently, because
> the notation of choice is excessively line-hungry.

This is only a matter of personal preference. It doesn´t interfere in any way with the review.

> 
> 
> Also, please:
> 
> C. Refrain from initial spaces/indentation in %description-s.

rationale?

> 
> D. Check whether there are some tests that could be run as part of
>    the build under %check scriptlet (to be added if that's the case).

this is a good point, but FYI upstream already has an extensive CI/CD including different versions of Fedora.

My only concern is that the testsuite does play with the network (loopback interface) and should be very safe, but in the unlucky event of bugs, we should probably avoid DoS´ing the fedora builders by generating unwanted traffic. I think that Digimer choice to avoid running the test suite is more of a safe precaution.

> 
> E. Because libknet1-devel requires (indirectly) libknet1, it may
>    drop the %license files, as these will be present thanks to
>    libknet1 installed in parallel, hence satisfying:
>   
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/
> LicensingGuidelines#Subpackage_Licensing

Is this a blocker for the review or a wishlist level? what are the consequences of not doing it?

> 
> 
> Rather for future consideration:
> - if the documentation for the API functions will keep growing,
>   it might be desirable to split those manpages to a separate
>   subpackage:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
> ("Or if there's a lot of documentation...")

Hardly, the API is pretty stable at this point.

>> Some concerns that remain:
>> 
>> A. [Comment 28] 4.: no reason to mangle with debuginfo generation
>> - one can always use command-line switches to achieve the same:
>>   http://lists.rpm.org/pipermail/rpm-list/2013-April/001416.html
>>   (definitely not a mainstream need, even less in Fedora context)
>> 
>> B. [Comment 11]: I'd still suggest using
>> 
>> %{configure} \
>>>   %{?with_sctp:--enable-libknet-sctp} \
>>>   %{!?with_sctp:--disable-libknet-sctp} \
>>> [...]
>> 
>> Reason is also practical, e.g. the whole "configure" statement
>> barely fits a single laptop screen for me currently, because
>> the notation of choice is excessively line-hungry.
>
> This is only a matter of personal preference. It doesn´t interfere
> in any way with the review.

Looks like bigger picture is neglected: having packages in somewhat
unsurprising state (goal of package review, afterall) is not to the
benefit of selected people, but for overall community.

https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility

Packages get routinely modified by proven packagers when
new packaging/technical needs arise.  And this general
eligibility/predictability applies to both above points.
Do not discount this by unsound personal preference claims, please.
It takes effort on both sides to undergo the review if the result
is not to become maintainance (and daily distro usage, e.g. in
case of ldconfig point H. below) burden anytime soon.

For instance, when a package attempts to juggle with debuginfo
generation on its own, it attracts attention needlessly, and it's
not clear at all why this is needed in Fedora context.  If you
think this is relevant for Fedora, the justification shall be
provided in the form of a comment.

Regarding using superfluous globals/non-terse conditionals, that
is something killing said general eligibility.  Instead of:

> %bcond_without sctp
> [...]
> %if %{with sctp}
> %global buildsctp 1
> %endif
> [...]
> %if %{defined buildsctp}
> BuildRequires: lksctp-tools-devel
> %endif
> [...]
> %{configure} \
> %if %{defined buildsctp}
>         --enable-libknet-sctp \
> %else
>         --disable-libknet-sctp \
> %endif

please use:

> %bcond_without sctp
> [...]
> %if %{with sctp}
> BuildRequires: lksctp-tools-devel
> %endif
> [...]
> %{configure} \
>   %{?with_sctp:--enable-libknet-sctp} \
>   %{!?with_sctp:--disable-libknet-sctp} \

[Voila, some bogus lines down, while eligibility raises.]

>> Also, please:
>> 
>> C. Refrain from initial spaces/indentation in %description-s.
>
> rationale?

Not looking weird in comparison to other packages, e.g. in
various output of console programs dealing with packages.
(see the above note on unsurprising state)

>> D. Check whether there are some tests that could be run as part of
>>    the build under %check scriptlet (to be added if that's the case).
>
> this is a good point, but FYI upstream already has an extensive CI/CD
> including different versions of Fedora.

This is indeed nice and respectable.  But to provide other use cases,
some preliminary toolchain rebuilds can be performed on the package
base in sole discretion of the people involved, and having some kind
of smoke test will help establish the confidence all work alright,
before even hitting Rawhide.  Also the number of architectures covered
this way is rather impressive.  And there are more benefits down the
road AIUI, like test-rebuilding the inverse dependencies, IOW when
some of kronosnet's dependency is receiving an update, for instance.

> My only concern is that the testsuite does play with the network
> (loopback interface) and should be very safe, but in the unlucky
> event of bugs, we should probably avoid DoS´ing the fedora builders
> by generating unwanted traffic. I think that Digimer choice to avoid
> running the test suite is more of a safe precaution.

Might be at least worth adding the %check scriptlet triggering some
straightforward test in a commented-out form together with a brief
explanation why it is deactivated per above.

>> E. Because libknet1-devel requires (indirectly) libknet1, it may
>>    drop the %license files, as these will be present thanks to
>>    libknet1 installed in parallel, hence satisfying:
>>   
>> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/
>> LicensingGuidelines#Subpackage_Licensing
>
> Is this a blocker for the review or a wishlist level? what are the
> consequences of not doing it?

Purpose of the package review is that at least at one well-defined
point the packaging is known to be in order and in-line with the
guidelines, otherwise it's mere relying on the eventual/hypothetical
settlement (for which COPR repositories exist).
In this light, let's just do it.

* * *

In addition, I have these (and no more, I swear) complaints:

F. only and/or and parentheses are meta-connectives for the license
   tag:

> GPLv2+ + LGPLv2+

   should become

> GPLv2+ and LGPLv2+

   https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios

G. regarding naming guidelines, explanation is missing why digit-ending
   packages (libknet1) are present, which is not customary in Fedora
   and only expected when multiple versions of a package can be
   installed simultaneously:
   https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:NamingGuidelines#MultiplePackages
   
   Is this indeed the case with kronosnet?  It doesn't look like that
   to me, otherwise the plugin dir (%{_libdir}/kronosnet) would also
   be versioned (e.g., %{_libdir}/kronosnet1).  Then, please drop
   those trailing digits from package names at all places.

H. instead of

> %post -n SUBPKG -p /sbin/ldconfig

   use

> %ldconfig_scriptlets SUBPKG

   the appropriate place:
   https://fedoraproject.org/wiki/Packaging:Scriptlets#Shared_Libraries

* * *

FYI only:

I. possible overlinking reported by rpmlint:

> libknet1.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libknet.so.1.1.0 /lib64/libm.so.6

J. "Suggests:" hints may be suitable for libknet on *-plugins-all
   ("Recommends:" will be understood in dnf case as "Requires:"
   by default, which may not be always desired, turning Suggests
   to Recommends looks less problematic than the opposite should
   the change become appealing)

... see
https://fedoraproject.org/wiki/Packaging:WeakDependencies

(In reply to Jan Pokorný from comment #57)
> >> Some concerns that remain:
> >> 
> >> A. [Comment 28] 4.: no reason to mangle with debuginfo generation
> >> - one can always use command-line switches to achieve the same:
> >>   http://lists.rpm.org/pipermail/rpm-list/2013-April/001416.html
> >>   (definitely not a mainstream need, even less in Fedora context)
> >> 
> >> B. [Comment 11]: I'd still suggest using
> >> 
> >> %{configure} \
> >>>   %{?with_sctp:--enable-libknet-sctp} \
> >>>   %{!?with_sctp:--disable-libknet-sctp} \
> >>> [...]
> >> 
> >> Reason is also practical, e.g. the whole "configure" statement
> >> barely fits a single laptop screen for me currently, because
> >> the notation of choice is excessively line-hungry.
> >
> > This is only a matter of personal preference. It doesn´t interfere
> > in any way with the review.
> 
> Looks like bigger picture is neglected: having packages in somewhat
> unsurprising state (goal of package review, afterall) is not to the
> benefit of selected people, but for overall community.
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility
> 
> Packages get routinely modified by proven packagers when
> new packaging/technical needs arise.  And this general
> eligibility/predictability applies to both above points.
> Do not discount this by unsound personal preference claims, please.
> It takes effort on both sides to undergo the review if the result
> is not to become maintainance (and daily distro usage, e.g. in
> case of ldconfig point H. below) burden anytime soon.
> 
> For instance, when a package attempts to juggle with debuginfo
> generation on its own, it attracts attention needlessly, and it's
> not clear at all why this is needed in Fedora context.  If you
> think this is relevant for Fedora, the justification shall be
> provided in the form of a comment.
> 
> Regarding using superfluous globals/non-terse conditionals, that
> is something killing said general eligibility.  Instead of:
> 
> > %bcond_without sctp
> > [...]
> > %if %{with sctp}
> > %global buildsctp 1
> > %endif
> > [...]
> > %if %{defined buildsctp}
> > BuildRequires: lksctp-tools-devel
> > %endif
> > [...]
> > %{configure} \
> > %if %{defined buildsctp}
> >         --enable-libknet-sctp \
> > %else
> >         --disable-libknet-sctp \
> > %endif
> 
> please use:
> 
> > %bcond_without sctp
> > [...]
> > %if %{with sctp}
> > BuildRequires: lksctp-tools-devel
> > %endif
> > [...]
> > %{configure} \
> >   %{?with_sctp:--enable-libknet-sctp} \
> >   %{!?with_sctp:--disable-libknet-sctp} \
> 
> [Voila, some bogus lines down, while eligibility raises.]

The current format still achieve the same technical goal. terse or verbose is still a matter of personal preference. The code is not obfuscated in either forms.

> 
> >> Also, please:
> >> 
> >> C. Refrain from initial spaces/indentation in %description-s.
> >
> > rationale?
> 
> Not looking weird in comparison to other packages, e.g. in
> various output of console programs dealing with packages.

Please provide an example of which commands are you referring to. It´s hard to guess what you see without some data.

> (see the above note on unsurprising state)
> 
> >> D. Check whether there are some tests that could be run as part of
> >>    the build under %check scriptlet (to be added if that's the case).
> >
> > this is a good point, but FYI upstream already has an extensive CI/CD
> > including different versions of Fedora.
> 
> This is indeed nice and respectable.  But to provide other use cases,
> some preliminary toolchain rebuilds can be performed on the package
> base in sole discretion of the people involved, and having some kind
> of smoke test will help establish the confidence all work alright,
> before even hitting Rawhide.  Also the number of architectures covered
> this way is rather impressive.  And there are more benefits down the
> road AIUI, like test-rebuilding the inverse dependencies, IOW when
> some of kronosnet's dependency is receiving an update, for instance.

the number of architectures is indeed a benefit, but we do test rebuild and check daily in CI to catch issues on latest of each distribution exactly for that use case. I am less interested in automatic rebuilds. Who does full rebuilds of fedora is never going to look at each single failure.

> 
> > My only concern is that the testsuite does play with the network
> > (loopback interface) and should be very safe, but in the unlucky
> > event of bugs, we should probably avoid DoS´ing the fedora builders
> > by generating unwanted traffic. I think that Digimer choice to avoid
> > running the test suite is more of a safe precaution.
> 
> Might be at least worth adding the %check scriptlet triggering some
> straightforward test in a commented-out form together with a brief
> explanation why it is deactivated per above.

Reasonable.

> 
> >> E. Because libknet1-devel requires (indirectly) libknet1, it may
> >>    drop the %license files, as these will be present thanks to
> >>    libknet1 installed in parallel, hence satisfying:
> >>   
> >> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/
> >> LicensingGuidelines#Subpackage_Licensing
> >
> > Is this a blocker for the review or a wishlist level? what are the
> > consequences of not doing it?
> 
> Purpose of the package review is that at least at one well-defined
> point the packaging is known to be in order and in-line with the
> guidelines, otherwise it's mere relying on the eventual/hypothetical
> settlement (for which COPR repositories exist).
> In this light, let's just do it.

You haven´t answered either of my questions.

> 
> * * *
> 
> In addition, I have these (and no more, I swear) complaints:
> 
> F. only and/or and parentheses are meta-connectives for the license
>    tag:
> 
> > GPLv2+ + LGPLv2+
> 
>    should become
> 
> > GPLv2+ and LGPLv2+

ok.

> 
>   
> https://fedoraproject.org/wiki/Packaging:
> LicensingGuidelines#Multiple_Licensing_Scenarios
> 
> G. regarding naming guidelines, explanation is missing why digit-ending
>    packages (libknet1) are present, which is not customary in Fedora
>    and only expected when multiple versions of a package can be
>    installed simultaneously:
>   
> https://fedoraproject.org/wiki/Packaging:Naming?rd=Packaging:
> NamingGuidelines#MultiplePackages
>    
>    Is this indeed the case with kronosnet?  It doesn't look like that
>    to me, otherwise the plugin dir (%{_libdir}/kronosnet) would also
>    be versioned (e.g., %{_libdir}/kronosnet1).  Then, please drop
>    those trailing digits from package names at all places.

It reflects the version of the onwire protocol. There are plenty libraries in Fedora that use similar convention. We can document it somewhere in the spec file. it´s not going to drop.

> 
> H. instead of
> 
> > %post -n SUBPKG -p /sbin/ldconfig
> 
>    use
> 
> > %ldconfig_scriptlets SUBPKG
> 
>    the appropriate place:
>    https://fedoraproject.org/wiki/Packaging:Scriptlets#Shared_Libraries
> 
> * * *
> 
> FYI only:
> 
> I. possible overlinking reported by rpmlint:
> 
> > libknet1.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libknet.so.1.1.0 /lib64/libm.so.6

which version of the package did you test? we fixed that already upstream in 1.1. ./configure.ac does check if it is necessary to link to libm or not at build time to use ceil().

> 
> J. "Suggests:" hints may be suitable for libknet on *-plugins-all
>    ("Recommends:" will be understood in dnf case as "Requires:"
>    by default, which may not be always desired, turning Suggests
>    to Recommends looks less problematic than the opposite should
>    the change become appealing)

> > I. possible overlinking reported by rpmlint:
> > 
> > > libknet1.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libknet.so.1.1.0 /lib64/libm.so.6
> 
> which version of the package did you test? we fixed that already upstream in
> 1.1. ./configure.ac does check if it is necessary to link to libm or not at
> build time to use ceil().
> 

Your rpm seems old.

Neither Honza´s rpmlint, or mine show that error:

[fabbione@lilith kronosnet]$ rpmlint libknet1-1.1-3.fc29.x86_64.rpm 
libknet1.x86_64: W: spelling-error Summary(en_US) Kronosnet -> Kronecker
libknet1.x86_64: W: spelling-error %description -l en_US Kronosnet -> Kronecker
libknet1.x86_64: W: spelling-error %description -l en_US knet -> net, knelt, knee
libknet1.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

tested with the latest brew build from Digimer and on all rpms generated by CI.

Created attachment 1402543 [details]
Example simplification

Slipped the recent comment, but mentioned in [comment 54]:
- either do not refer to particular macros in the changelog
  at all, double '%' characters (producing single '%' after
  macro processing, preventing macro expansion as such when
  it happens during the build process), or put it in some other
  understandable way, e.g.: "_isa" macro

* * *

>> [re A. and B.[
> 
> The current format still achieve the same technical goal. terse or
> verbose is still a matter of personal preference. The code is not
> obfuscated in either forms.

We still did not get to why you insist on avoidable (B.) or
inappropriate (A.) complexities without any gain for Fedora
ecosystem.

Attached is the patch how it may look.


>>>> Also, please:
>>>> 
>>>> C. Refrain from initial spaces/indentation in %description-s.
>>>
>>> rationale?
>> 
>> Not looking weird in comparison to other packages, e.g. in
>> various output of console programs dealing with packages.
>
> Please provide an example of which commands are you referring to.
> It´s hard to guess what you see without some data.

"rpm -qi" is the first test of choice.


> [%check scriptlet discussion]

Thanks.


>> [re E.]
> 
> You haven´t answered either of my questions.

My answer was: let's just do it.


>> [re G.]
> 
> It reflects the version of the onwire protocol. There are plenty
> libraries in Fedora that use similar convention. We can document
> it somewhere in the spec file. it´s not going to drop.

True, and other library cases mainly boil down to "multiple versions
installed simulatenously" scenario as shown.  Explicitly designating
on-wire compatible series may also make sense, then please make this
apparent in package summaries, e.g.:

-Summary: Kronosnet core switching implementation 
+Summary: Kronosnet core switching implementation (protocol v1)


>> [re I.]
> 
> which version of the package did you test? we fixed that already
> upstream in 1.1. ./configure.ac does check if it is necessary to link
> to libm or not at build time to use ceil().
> 
> Your rpm seems old.

Used SRPM per [comment 52], using.  Just rechecked.
It can be found in "Rpmlint (installed packages)" of review.txt
generated with "fedora-review -rn kronosnet-1.1-3.fc27.src.rpm"
on Rawhide.

As mentioned, this has no effect on the review itself, just
a feedback about detections (allowing for false positives) observed.

(In reply to Jan Pokorný from comment #61)
> Created attachment 1402543 [details]
> Example simplification
> 
> Slipped the recent comment, but mentioned in [comment 54]:
> - either do not refer to particular macros in the changelog
>   at all, double '%' characters (producing single '%' after
>   macro processing, preventing macro expansion as such when
>   it happens during the build process), or put it in some other
>   understandable way, e.g.: "_isa" macro
> 
> * * *
> 
> >> [re A. and B.[
> > 
> > The current format still achieve the same technical goal. terse or
> > verbose is still a matter of personal preference. The code is not
> > obfuscated in either forms.
> 
> We still did not get to why you insist on avoidable (B.) or
> inappropriate (A.) complexities without any gain for Fedora
> ecosystem.
> 

Because it´s impossible to follow those jumps you do around between comments and we have no idea what you are talking about related to A. What is A? How about adding some context or spec file snippets to make it clear?

If B is the configure switches, you need to learn to discern what is required for a review or nice to have. You mentioned different times that it is nice to have and since there are no fedora strict requirements to switch to your format of choice, then I don´t see why we have to change it.

> Attached is the patch how it may look.
> 
> 
> >>>> Also, please:
> >>>> 
> >>>> C. Refrain from initial spaces/indentation in %description-s.
> >>>
> >>> rationale?
> >> 
> >> Not looking weird in comparison to other packages, e.g. in
> >> various output of console programs dealing with packages.
> >
> > Please provide an example of which commands are you referring to.
> > It´s hard to guess what you see without some data.
> 
> "rpm -qi" is the first test of choice.

Ok now I can see it.

> 
> 
> > [%check scriptlet discussion]
> 
> Thanks.
> 
> 
> >> [re E.]
> > 
> > You haven´t answered either of my questions.
> 
> My answer was: let's just do it.

My questions are still unanswered. I asked if it is breaking policies or not.

> 
> 
> >> [re G.]
> > 
> > It reflects the version of the onwire protocol. There are plenty
> > libraries in Fedora that use similar convention. We can document
> > it somewhere in the spec file. it´s not going to drop.
> 
> True, and other library cases mainly boil down to "multiple versions
> installed simulatenously" scenario as shown.  Explicitly designating
> on-wire compatible series may also make sense, then please make this
> apparent in package summaries, e.g.:
> 
> -Summary: Kronosnet core switching implementation 
> +Summary: Kronosnet core switching implementation (protocol v1)

ok that´s reasonable.

> 
> 
> >> [re I.]
> > 
> > which version of the package did you test? we fixed that already
> > upstream in 1.1. ./configure.ac does check if it is necessary to link
> > to libm or not at build time to use ceil().
> > 
> > Your rpm seems old.
> 
> Used SRPM per [comment 52], using.  Just rechecked.
> It can be found in "Rpmlint (installed packages)" of review.txt
> generated with "fedora-review -rn kronosnet-1.1-3.fc27.src.rpm"
> on Rawhide.

hmmm does fedora-review -rn rebuilds the package on the local system (aka f29?) or does it build in a chroot for f27 and test the end result.f27 binaries?


> 
> As mentioned, this has no effect on the review itself, just
> a feedback about detections (allowing for false positives) observed.

Right, that´s clear. I am investigating because it annoys me.

libknet uses ceil(). ceil() recently moved from libm to libc. The build system might detect that it needs to link to libm but then when you do rpmlint on a different system that has ceil() in libc, the symbol check would report the warning.

So I prefer to understand if the upstream check is bogus on fedora or if the test environment (despite being just a minor warning) is being tricked to think it´s a problem.

> Because it´s impossible to follow those jumps

Really?  Enumerating separate points was to prevent any confusion.

> you do around between comments and we have no idea what you are
> talking about related to A.  What is A?

For whoever is "we", A. was defined in [comment 55]...

> How about adding some context or spec file snippets to make
> it clear?

...see [attachment 1402543 [details]], lines 25,26, 28-30, 458-460 in the
original.  Hopefully it clarifies it fully.


>>>> [re E.]
>>> 
>>> You haven´t answered either of my questions.
>> 
>> My answer was: let's just do it.
>
> My questions are still unanswered. I asked if it is breaking
> policies or not.

Let me explain that the review is not a black-or-white
mechanical process, which appears to be your current view.

It's meant to combine experience and common sense to find
near-optimal solutions, individually.

In this case, the undisclosed reason behind my suggestion is the common
sense one: to eliminate redundancy.  To keep Fedora scalable regarding
mirrors, minimal installations, etc., it's really good to consider
whether the identical files need to be carried over twice per each
architecture when not necessary.  Common sense hence says, it's enough
-- thanks to intra-srpm dependencies -- to carry just a single copy
per arch.  Packaging guidelines give the green light to this
"optimization".  I see I should have been more patient to provide
this explanation first-hand, sorry about that.

Would you be willing to make this change, i.e., to drop license files
from libknet1-devel subpackage, now?

* * *

> hmmm does fedora-review -rn rebuilds the package on the local system
> (aka f29?) or does it build in a chroot for f27 and test the end
> result.f27 binaries?

It built the package detached from host, using mock (systemd-nspawn
containers), which in turn obtained buildroot with
> /usr/bin/dnf builddep --installroot \
> /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 28 \
> /var/lib/mock/fedora-rawhide-x86_64/root//builddir/build/SRPMS/kronosnet-1.1-3.fc28.src.rpm

with toolchain:
> gcc-8.0.1-0.14.fc28.x86_6
> binutils-2.29.1-19.fc28.x86_64
> glibc-2.27-3.fc28.x86_64

Then

> So I prefer to understand if the upstream check is bogus on fedora or
> if the test environment (despite being just a minor warning) is being
> tricked to think it´s a problem.

in the respective logs, I can see:
> checking for library containing ceil... -lm

It corresponds to this observation:
$ readelf -s /usr/lib64/libc.so.6  | grep ceil || echo none
> none

So the warning may be false positive, after all.

(In reply to Jan Pokorný from comment #63)
> > Because it´s impossible to follow those jumps
> 
> Really?  Enumerating separate points was to prevent any confusion.

Yes really.

> 
> > you do around between comments and we have no idea what you are
> > talking about related to A.  What is A?
> 
> For whoever is "we", A. was defined in [comment 55]...


"we" are all the people currently involved in this review to move this package forward.

> 
> > How about adding some context or spec file snippets to make
> > it clear?
> 
> ...see [attachment 1402543 [details]], lines 25,26, 28-30, 458-460 in the
> original.  Hopefully it clarifies it fully.
> 

was it that hard to reference the original lines?

The debuginfo generation is default: on in fedora. Those statements have no effect on fedora unless explicitly overridden. Those are coming from upstream spec file that requires tuning to build debuginfo on Opensuse. They create no harm and have no effect on Fedora. The end result is the same.

Something you missed in the process is that as the review goes, we are merging all those bits back into upstream spec file so that it can build properly both for suse and fedora / rhel / centos and reduce the need to maintain multiple spec files around (after all as you somehow agreed below in another context we want to kill redundancy).

Given that those are more useful on opensuse we can add a:
%if 0%{?suse_version}
somewhere later on to make them even more transparent for fedora.

Let´s move on.

> 
> >>>> [re E.]
> >>> 
> >>> You haven´t answered either of my questions.
> >> 
> >> My answer was: let's just do it.
> >
> > My questions are still unanswered. I asked if it is breaking
> > policies or not.
> 
> Let me explain that the review is not a black-or-white
> mechanical process, which appears to be your current view.
> 
> It's meant to combine experience and common sense to find
> near-optimal solutions, individually.

Oh finally you start talking like a real mentor that a good reviewer should be.

> 
> In this case, the undisclosed reason behind my suggestion is the common
> sense one: to eliminate redundancy.  To keep Fedora scalable regarding
> mirrors, minimal installations, etc., it's really good to consider
> whether the identical files need to be carried over twice per each
> architecture when not necessary.  Common sense hence says, it's enough
> -- thanks to intra-srpm dependencies -- to carry just a single copy
> per arch.  Packaging guidelines give the green light to this
> "optimization".  I see I should have been more patient to provide
> this explanation first-hand, sorry about that.
> 
> Would you be willing to make this change, i.e., to drop license files
> from libknet1-devel subpackage, now?

Yes of course, now that you provided a good rationale on your request, I have no objections to make changes.

> 
> * * *
> 
> > hmmm does fedora-review -rn rebuilds the package on the local system
> > (aka f29?) or does it build in a chroot for f27 and test the end
> > result.f27 binaries?
> 
> It built the package detached from host, using mock (systemd-nspawn
> containers), which in turn obtained buildroot with
> > /usr/bin/dnf builddep --installroot \
> > /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 28 \
> > /var/lib/mock/fedora-rawhide-x86_64/root//builddir/build/SRPMS/kronosnet-1.1-3.fc28.src.rpm
> 
> with toolchain:
> > gcc-8.0.1-0.14.fc28.x86_6
> > binutils-2.29.1-19.fc28.x86_64
> > glibc-2.27-3.fc28.x86_64
> 
> Then
> 
> > So I prefer to understand if the upstream check is bogus on fedora or
> > if the test environment (despite being just a minor warning) is being
> > tricked to think it´s a problem.
> 
> in the respective logs, I can see:
> > checking for library containing ceil... -lm
> 
> It corresponds to this observation:
> $ readelf -s /usr/lib64/libc.so.6  | grep ceil || echo none
> > none
> 
> So the warning may be false positive, after all.

Ok, that makes sense.

[re A.]

> The debuginfo generation is default: on in fedora. Those statements
> have no effect on fedora unless explicitly overridden. Those are
> coming from upstream spec file that requires tuning to build debuginfo
> on Opensuse.

Can you enlighten me, then, how OpenSUSE and their OBS perhaps relate
to the need to specify "debug_package" macro explicitly?  I was unable
to find such instructions, and the spec-s I looked at did not need it,
either, e.g.:
https://build.opensuse.org/package/view_file/devel:gcc/gcc8/gcc8.spec?expand=1

> They create no harm and have no effect on Fedora.  The end result is
> the same.

I am not disputing direct effects, just the spec file clarity
important for comprehension by arbitrary Fedora maintainers, per the
linked statement in the guidelines:
https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility

Anything not a concern of Fedora doesn't belong to a dedicated specfile.

> Something you missed in the process is that as the review goes, we are
> merging all those bits back into upstream spec file so that it can
> build properly both for suse and fedora / rhel / centos and reduce the
> need to maintain multiple spec files around (after all as you somehow
> agreed below in another context we want to kill redundancy).
> 
> Given that those are more useful on opensuse we can add a:
> %if 0%{?suse_version}
> somewhere later on to make them even more transparent for fedora.

This is expressly forbidden, see the link.

Therein lies a clear conflict of interest:

- upstream: one-size-fits-all should it be interested in high-level
            packaging at all

- downstream: well-tailored solution, following special needs and
              conventions of the distribution for its greater good

Solution may be a mix of:

- use conditionalized spec in upstream, drop irrelevant conditionals
  when reflecting the upstream changes in downstream

- maintain downstream-quality spec files in upstream as discrete files
  per distro

- synthesis of the previous two can be to use a macro language like
  M4 to conditionalize without spoiling the results (for clufter,
  I abused the fact that spec language is in itself based on expanding
  macros, which can be selectively blinded with external preprocessing:
  https://pagure.io/distill-spec, hence can have upstream spec file
  almost arbitrarily complex, but that's just a source for further
  chewing: https://pagure.io/clufter/blob/master/f/misc/clufter.spec)

- apply nifty tricks, which we did for instance in pacemaker to
  support %license tag also in EL6:
  https://github.com/ClusterLabs/pacemaker/commit/a592019fbe88144bc42131b0e20deea96acd6d45

(In reply to Jan Pokorný from comment #65)
> [re A.]
> 
> > The debuginfo generation is default: on in fedora. Those statements
> > have no effect on fedora unless explicitly overridden. Those are
> > coming from upstream spec file that requires tuning to build debuginfo
> > on Opensuse.
> 
> Can you enlighten me, then, how OpenSUSE and their OBS perhaps relate
> to the need to specify "debug_package" macro explicitly?  I was unable
> to find such instructions, and the spec-s I looked at did not need it,
> either, e.g.:
> https://build.opensuse.org/package/view_file/devel:gcc/gcc8/gcc8.
> spec?expand=1

Not sure, we received the patch from the Suse maintainers, asking explicitly to enable it. Given that it didn´t affect Fedora I didn´t feel the need to investigate further.

Probably the OBS is not the same as internal OpenSUSE build system?

> 
> > They create no harm and have no effect on Fedora.  The end result is
> > the same.
> 
> I am not disputing direct effects, just the spec file clarity
> important for comprehension by arbitrary Fedora maintainers, per the
> linked statement in the guidelines:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_Legibility
> 
> Anything not a concern of Fedora doesn't belong to a dedicated specfile.

see below.

> 
> > Something you missed in the process is that as the review goes, we are
> > merging all those bits back into upstream spec file so that it can
> > build properly both for suse and fedora / rhel / centos and reduce the
> > need to maintain multiple spec files around (after all as you somehow
> > agreed below in another context we want to kill redundancy).
> > 
> > Given that those are more useful on opensuse we can add a:
> > %if 0%{?suse_version}
> > somewhere later on to make them even more transparent for fedora.
> 
> This is expressly forbidden, see the link.

the statement can be somehow interpreted:

"To help facilitate legibility, only macros and conditionals for Fedora and EPEL are allowed to be used in Fedora Packages. Use of macros and conditionals for other distributions, including Fedora derivatives, is not permitted in spec files of packages in the main Fedora repositories unless those macros and conditionals are also present in Fedora."

Then we can just change them to %if 0%{?fedora*..} but before answering, please read more below first.

> 
> Therein lies a clear conflict of interest:
> 
> - upstream: one-size-fits-all should it be interested in high-level
>             packaging at all
> 
> - downstream: well-tailored solution, following special needs and
>               conventions of the distribution for its greater good
> 
> Solution may be a mix of:
> 
> - use conditionalized spec in upstream, drop irrelevant conditionals
>   when reflecting the upstream changes in downstream
> 
> - maintain downstream-quality spec files in upstream as discrete files
>   per distro
> 
> - synthesis of the previous two can be to use a macro language like
>   M4 to conditionalize without spoiling the results (for clufter,
>   I abused the fact that spec language is in itself based on expanding
>   macros, which can be selectively blinded with external preprocessing:
>   https://pagure.io/distill-spec, hence can have upstream spec file
>   almost arbitrarily complex, but that's just a source for further
>   chewing: https://pagure.io/clufter/blob/master/f/misc/clufter.spec)
> 
> - apply nifty tricks, which we did for instance in pacemaker to
>   support %license tag also in EL6:
>  
> https://github.com/ClusterLabs/pacemaker/commit/
> a592019fbe88144bc42131b0e20deea96acd6d45

The technicality on how to get there is an upstream problem, but ideally we will get the make $distro-specfile upstream to generate a valid (and policy compliant) spec file.

One step at a time tho, once this is done, we can start merging things upstream for downstream benefit.

> Not sure, we received the patch from the Suse maintainers, asking
> explicitly to enable it. Given that it didn´t affect Fedora I didn´t
> feel the need to investigate further.
> 
> Probably the OBS is not the same as internal OpenSUSE build system?

https://github.com/kronosnet/kronosnet/pull/98#issuecomment-369880485


Btw. I am still thinking how the future protocol bumps will work out,
shouldn't the pkgconfig file contain the versioning in its name
as well (see dbus, glib, etc.)?  If so, it would be preferred to make
that change prior to inclusion (so that no bogus pkgconfig virtual
provides get spread).

> 
> Btw. I am still thinking how the future protocol bumps will work out,
> shouldn't the pkgconfig file contain the versioning in its name
> as well (see dbus, glib, etc.)?  If so, it would be preferred to make
> that change prior to inclusion (so that no bogus pkgconfig virtual
> provides get spread).

whoever is going to use libknet will have BR libknet1-devel or libknetX-devel and Requires the equivalent. the BR is explicit.

But the client programs will likely use the pkgconfig dependency in the
build setup, and that has to be differentiated eventually in case it
cares about the protocol version.

> PKG_CHECK_MODULES(DBUS, libknet1, ...)

would make the specification targeted per the least surprise principle.
Otherwise versioning subpackages directly in the name is just half-baked
anticipation of future progress.

And then, RPM is automatically picking any pkgconfig files, turning
them into virtual "pkgconfig(X)" provides, i.e., fixed point in the
package dependencies graph (it was used to avoid file-based dependencies
hinted in [comment 28]), which is a concern from packaging perspective.

(In reply to Jan Pokorný from comment #69)
> But the client programs will likely use the pkgconfig dependency in the
> build setup, and that has to be differentiated eventually in case it
> cares about the protocol version.

The protocol is completely transparent to the final application. The API doesn´t allow mingling of the onwire protocol at any level.

All the application cares is the API (and features) related to a given release.

the pkg-config file correctly exports a Version: already. If an application needs newer versions they can rely on that at configure time.

Ok, the same argument can be applied to implicit versioning of
subpackages (BuildRequires: libknet-devel%{?_isa} < 2.0), why
do you want to treat these two things (subpackages and respective
pkgconfig files) differently, especially (to repeat it) if it's
customary for the latter even when some packages (dbus) do
explicit versioning for the former in addition (dbus-1.pc while
avoiding dbus1-devel as the name of a subpackage)?

Am I the only to see a conflict here?

Will hypothetical libknet2 ship its standalone libknet2.pc?
Why not to apply unified approach and rename libknet.pc to
libknet1.pc.  Or conversely, to stop the explicit versioning
in the subpackage names in there's ever to be just a single
pkgconfig file...

(In reply to Jan Pokorný from comment #71)
> Ok, the same argument can be applied to implicit versioning of
> subpackages (BuildRequires: libknet-devel%{?_isa} < 2.0), why
> do you want to treat these two things (subpackages and respective
> pkgconfig files) differently, especially (to repeat it) if it's
> customary for the latter even when some packages (dbus) do
> explicit versioning for the former in addition (dbus-1.pc while
> avoiding dbus1-devel as the name of a subpackage)?
> 
> Am I the only to see a conflict here?

I honestly don´t see the problem. One is upstream way to express versioning and one is packaging. Each distro has its own similar but different ways to handle it.

> 
> Will hypothetical libknet2 ship its standalone libknet2.pc?

No, it will ship libknet.pc, I don´t want or expect that v1 or v2 can be co-installed or co-exist in the same system.

> Why not to apply unified approach and rename libknet.pc to
> libknet1.pc.  Or conversely, to stop the explicit versioning
> in the subpackage names in there's ever to be just a single
> pkgconfig file...

I already explain why the libknet1 should have the number there to express protocol version being installed/used in that build.

Ok, if the expectations are set like this, meaning that the future
obstacles I was worried about -- mostly related to parallel pkgconfig
files as their names form de facto inter-dependencies parallel of
API in RPM world (hence something that should be established wisely
since the beginning because once the client packages will start to
pick this "pkgconfig(libknet)", Pandora's box is open and maintenance
burden cannot be taken back) -- won't occur (all libknet clients will
need to be rebuilt for/ported to libknet2 at once on the single system,
and all the systems they want to communicate with unless compatibility
is preserved), I will conclude this extension of point G. with a simple
task to have in-spec comment above "%files -n libknet1-devel" to
express this explicitly, e.g.:

> # libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
> # like other files, is not explicitly versioned in the name like the
> # subpackages are -- intention of doing so for subpackage names is
> # to ease the cross-checking the compatibility of the remote clients
> # interchanging data using this network communication library, as
> # the number denotes the protocol version (providing multiple
> # protocol versions in parallel is not planned).
> %files -n libknet1-devel

[I hope I picked the gist of the reasoning right, but really can't see
any other justification, to version packaged libraries like this all
the time may be common for Debian, but this is not Debian, hopefully
this is clear.]

This is in addition to summary tags per [comment 61].

* * *

Regarding A., I can tolerate the situation as is provided there's:

- clear promise to do something about that in the future
  (I will follow-up on that github question to see if the original
  use case cannot be handled by other means, which would be the
  simplest solution, otherwise there are these casing options etc.
  to be implemented if upstream-downstream sync is required)

- comment above "%debug_package" that it is not relevant for
  Fedora and its removal is pending

* * *

Please, present the fixed spec file so I can recheck and approve
the review.

(In reply to Jan Pokorný from comment #73)
> Ok, if the expectations are set like this, meaning that the future
> obstacles I was worried about -- mostly related to parallel pkgconfig
> files as their names form de facto inter-dependencies parallel of
> API in RPM world (hence something that should be established wisely
> since the beginning because once the client packages will start to
> pick this "pkgconfig(libknet)", Pandora's box is open and maintenance
> burden cannot be taken back) -- won't occur (all libknet clients will
> need to be rebuilt for/ported to libknet2 at once on the single system,
> and all the systems they want to communicate with unless compatibility
> is preserved), I will conclude this extension of point G. with a simple
> task to have in-spec comment above "%files -n libknet1-devel" to
> express this explicitly, e.g.:
> 
> > # libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
> > # like other files, is not explicitly versioned in the name like the
> > # subpackages are -- intention of doing so for subpackage names is
> > # to ease the cross-checking the compatibility of the remote clients
> > # interchanging data using this network communication library, as
> > # the number denotes the protocol version (providing multiple
> > # protocol versions in parallel is not planned).
> > %files -n libknet1-devel

Good enough explanation.

> 
> [I hope I picked the gist of the reasoning right, but really can't see
> any other justification, to version packaged libraries like this all
> the time may be common for Debian, but this is not Debian, hopefully
> this is clear.]

Let´s not mix up things please. Debian uses the soname there.

> 
> This is in addition to summary tags per [comment 61].
> 
> * * *
> 
> Regarding A., I can tolerate the situation as is provided there's:
> 
> - clear promise to do something about that in the future
>   (I will follow-up on that github question to see if the original
>   use case cannot be handled by other means, which would be the
>   simplest solution, otherwise there are these casing options etc.
>   to be implemented if upstream-downstream sync is required)
> 
> - comment above "%debug_package" that it is not relevant for
>   Fedora and its removal is pending

ack.

> 
> * * *
> 
> Please, present the fixed spec file so I can recheck and approve
> the review.

There has been a lot of back and forth, but I think I got most of it. Please review and let me know if anything is still overlooked/outstanding.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-4
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-4.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25463902

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25463917

f28:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25463942

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25463975

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25463987


Diff from 1.1-3:
====
--- kronosnet.spec.1.1-3	2018-02-26 10:03:13.304992918 -0500
+++ kronosnet.spec.1.1-4	2018-03-04 02:00:23.290918796 -0500
@@ -70,8 +70,8 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.1
-Release: 3%{?dist}
-License: GPLv2+ + LGPLv2+
+Release: 4%{?dist}
+License: GPLv2+ and LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
 
@@ -190,6 +190,13 @@
 # remove docs
 rm -rf %{buildroot}/usr/share/doc/kronosnet
 
+# Disabled because of concern that the testsuite does not play nice with the 
+# network loopback interface. Upstream has a comprehensive CI/CD system which
+# tests different versions of Fedora and should be very safe. In the unlikely
+# event of bugs, we should probably avoid DoS´ing the fedora builders by 
+# generating unwanted traffic.
+#%check
+
 # main empty package
 %description
 kronosnet source
@@ -205,14 +212,14 @@
 
 %description -n kronosnetd
 The kronosnet daemon is a bridge between kronosnet switching engine
- and kernel network tap devices, to create and administer a
- distributed LAN over multipoint-to-multipoint VPNs.
+and kernel network tap devices, to create and administer a
+distributed LAN over multipoint-to-multipoint VPNs.
  
- The daemon does a poor attempt to provide a configure UI similar
- to other known network devices/tools (Cisco, quagga).
- Beside looking horrific, it allows runtime changes and
- reconfiguration of the kronosnet(s) without daemon reload
- or service disruption.
+The daemon does a poor attempt to provide a configure UI similar
+to other known network devices/tools (Cisco, quagga).
+Beside looking horrific, it allows runtime changes and
+reconfiguration of the kronosnet(s) without daemon reload
+or service disruption.
 
 %post -n kronosnetd
 %systemd_post kronosnetd.service
@@ -242,15 +249,15 @@
 License: LGPLv2+
 
 %description -n libtap1
- This is an over-engineered commodity library to manage a pool
- of tap devices and provides the basic
- pre-up.d/up.d/down.d/post-down.d infrastructure.
+This is an over-engineered commodity library to manage a pool
+of tap devices and provides the basic
+pre-up.d/up.d/down.d/post-down.d infrastructure.
 
 %files -n libtap1
 %license COPYING.* COPYRIGHT
 %{_libdir}/libtap.so.*
 
-%post -n libtap1 -p /sbin/ldconfig
+%ldconfig_scriptlets libtap1
 
 %postun -n libtap1 -p /sbin/ldconfig
 
@@ -261,9 +268,9 @@
 Requires: pkgconfig
 
 %description -n libtap1-devel
- This is an over-engineered commodity library to manage a pool
- of tap devices and provides the basic
- pre-up.d/up.d/down.d/post-down.d infrastructure.
+This is an over-engineered commodity library to manage a pool
+of tap devices and provides the basic
+pre-up.d/up.d/down.d/post-down.d infrastructure.
 
 %files -n libtap1-devel
 %license COPYING.* COPYRIGHT
@@ -273,19 +280,19 @@
 %endif
 
 %package -n libknet1
-Summary: Kronosnet core switching implementation
+Summary: Kronosnet core switching implementation (protocol v1)
 License: LGPLv2+
 BuildRequires: libqb-devel
 BuildRequires: doxygen
 
 %description -n libknet1
- Kronosnet, often referred to as knet, is a network abstraction layer 
- designed for High Availability use cases, where redundancy, security, 
- fault tolerance and fast fail-over are the core requirements of your 
- application.
+Kronosnet, often referred to as knet, is a network abstraction layer 
+designed for High Availability use cases, where redundancy, security, 
+fault tolerance and fast fail-over are the core requirements of your 
+application.
 
- The whole kronosnet core is implemented in this library.
- Please refer to https://kronosnet.org/ for further  information.
+The whole kronosnet core is implemented in this library.
+Please refer to https://kronosnet.org/ for further  information.
 
 %files -n libknet1
 %license COPYING.* COPYRIGHT
@@ -303,12 +310,18 @@
 Requires: pkgconfig
 
 %description -n libknet1-devel
- The whole kronosnet core is implemented in this library.
- Please refer to the not-yet-existing documentation for further
- information. 
-
+The whole kronosnet core is implemented in this library.
+Please refer to the not-yet-existing documentation for further
+information. 
+
+# libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
+# like other files, is not explicitly versioned in the name like the
+# subpackages are -- intention of doing so for subpackage names is
+# to ease the cross-checking the compatibility of the remote clients
+# interchanging data using this network communication library, as
+# the number denotes the protocol version (providing multiple
+# protocol versions in parallel is not planned).
 %files -n libknet1-devel
-%license COPYING.* COPYRIGHT
 %{_libdir}/libknet.so
 %{_includedir}/libknet.h
 %{_libdir}/pkgconfig/libknet.pc
@@ -321,7 +334,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-crypto-nss-plugin
- NSS crypto support for libknet1.
+NSS crypto support for libknet1.
 
 %files -n libknet1-crypto-nss-plugin
 %{_libdir}/kronosnet/crypto_nss.so
@@ -334,7 +347,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-crypto-openssl-plugin
- OpenSSL crypto support for libknet1.
+OpenSSL crypto support for libknet1.
 
 %files -n libknet1-crypto-openssl-plugin
 %{_libdir}/kronosnet/crypto_openssl.so
@@ -347,7 +360,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-zlib-plugin
- zlib compression support for libknet1.
+zlib compression support for libknet1.
 
 %files -n libknet1-compress-zlib-plugin
 %{_libdir}/kronosnet/compress_zlib.so
@@ -359,7 +372,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lz4-plugin
- lz4 and lz4hc compression support for libknet1.
+lz4 and lz4hc compression support for libknet1.
 
 %files -n libknet1-compress-lz4-plugin
 %{_libdir}/kronosnet/compress_lz4.so
@@ -373,7 +386,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lzo2-plugin
- lzo2 compression support for libknet1.
+lzo2 compression support for libknet1.
 
 %files -n libknet1-compress-lzo2-plugin
 %{_libdir}/kronosnet/compress_lzo2.so
@@ -386,7 +399,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-lzma-plugin
- lzma compression support for libknet1.
+lzma compression support for libknet1.
 
 %files -n libknet1-compress-lzma-plugin
 %{_libdir}/kronosnet/compress_lzma.so
@@ -399,7 +412,7 @@
 Requires: libknet1%{_isa} = %{version}-%{release}
 
 %description -n libknet1-compress-bzip2-plugin
- bzip2 compression support for libknet1.
+bzip2 compression support for libknet1.
 
 %files -n libknet1-compress-bzip2-plugin
 %{_libdir}/kronosnet/compress_bzip2.so
@@ -416,7 +429,7 @@
 %endif
 
 %description -n libknet1-crypto-plugins-all
- meta package to install all of libknet1 crypto plugins
+meta package to install all of libknet1 crypto plugins
 
 %files -n libknet1-crypto-plugins-all
 
@@ -440,7 +453,7 @@
 %endif
 
 %description -n libknet1-compress-plugins-all
- meta package to install all of libknet1 compress plugins
+meta package to install all of libknet1 compress plugins
 
 %files -n libknet1-compress-plugins-all
 
@@ -451,7 +464,7 @@
 Requires: libknet1-crypto-plugins-all%{_isa} = %{version}-%{release}
 
 %description -n libknet1-plugins-all
- meta package to install all of libknet1 plugins
+meta package to install all of libknet1 plugins
 
 %files -n libknet1-plugins-all
 
@@ -460,6 +473,15 @@
 %endif
 
 %changelog
+* Sun Mar 04 2018 Madison Kelly <mkelly> - 1.1-4
+- Removed leading spaces from descriptions.
+- Added the (commented out) %%check tests.
+- Updated the changelog macro references to have two percent signs.
+- Dropped the redundant libknet1-devel license files.
+- Changed 'GPLv2+ + LGPLv2+' to 'GPLv2+ and LGPLv2+'.
+- Updated %%ldconfig_scriptlets call.
+- Clarified the kronosnet protocol version in the summary. 
+
 * Mon Feb 26 2018 Madison Kelly <mkelly> - 1.1-3
 - Fixed the changelog to not have the full macro names.
 
@@ -477,7 +499,7 @@
 * Thu Feb 22 2018 Madison Kelly <mkelly> - 1.0-9
 - Changed the source tarball to be one from the upstream source.
 - Updated the main license.
-- Moved down the {?systemd_requires} macro.
+- Moved down the %%{?systemd_requires} macro.
 
 * Fri Feb 16 2018 Madison Kelly <mkelly> - 1.0-8
 - Reverted to 'BuildRequires: bzip2-devel' to fix EPEL7 builds.
@@ -493,7 +515,7 @@
 - Removed sysvinit checks.
 - Fixed the groupadd to add to system group.
 - Added build requirement for systemd.
-- Added {_isa} macro to knet requirements in sub packages.
+- Added %%{_isa} macro to knet requirements in sub packages.
 
 * Mon Feb 12 2018 Madison Kelly <mkelly> - 1.0-5
 - All changes related to this spec;
====

Thanks to everyone involved, also for patience; I understand that
time as a main metric might be preferred to overall quality, but
I tried to explain the significance of the latter for the wider
"package collection".  As this is one-off currently, it's believed
that the reviewed version will be used as a gauge for future updates.

That being said, I find just two points to be addressed prior to
approval, and I am at least partially responsible for the first one:

* * *

re H.:
I should have explained better the required change, as currently
it's only partially satisfied (beside following bad instructions).

Let me provide wider context:

- until very recently, /sbin/ldconfig invocations were interspersed
  in individual spec files in %post and %postun scriptlets (execution
  unit triggered as the package is being installed/removed in this
  case), to keep the dynamic linker cached knowledge about the
  installed libraries current -- so far so good, this is what previous
  version of this spec was using

- some time ago, rpm added generic support for scriptlets to be run
  only once per whole transaction (comprising, e.g., all your updates
  received in one go)

- relatively recently, wise Fedora maintainers realized there can be
  cumulated overhead arising from running ldconfig per package, when
  in most cases it's enough to run it per said transaction, and
  introduced new "self contained" change:

  https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets

  which asks either to drop ldconfig invocation from the spec
  altogether (Fedora 28+ only), or to use the compatibility
  macros (to cover also pre-28 cases, which I believe is the
  intention with kronosnet)

The problem is that we need to address all "ldconfig" occurrences,
not just the one I've mistakenly mentioned in isolation in
[comment 57].  Beside the description at the above page, we can
also investigate on our own:

$ rpm --showrc | grep -A3 ldconfig_scriptlets
> -13: ldconfig_scriptlets(n:)	%{?ldconfig:
> %ldconfig_post %{?*} %{-n:-n %{-n*}}
> %ldconfig_postun %{?*} %{-n:-n %{-n*}}
> }

We can grok that %ldconfig_scriptlets will handle both %post
and %postun sides on its own.  Regarding usage with subpackages,
there's a confirmation that currently adopted usage is OK, e.g.,
on devel ML:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/JZNJEN5FG5SAVQKC5RTWEJDQDLBETNI4/

Hence what I ask for: for both libtap1 and libknet1 subpackages,
make sure that these occurrences in the original:

> %post -n SUBPACKAGE -p /sbin/ldconfig
> %postun -n SUBPACKAGE -p /sbin/ldconfig

are both mapped to a single, unique line:

> %ldconfig_scriptlets -n SUBPACKAGE

As you can see, I mistakenly provided a bad piece of advice, actually,
as using "-n SUBPACKAGE" vs. just "SUBPACKAGE" is aligned with how
other macros (namely %package, but consequently %description, %files,
%postun, etc.), and it's the former case with both libknet1 and
libtap1.
These naming subtleties are best understood here:
http://ftp.rpm.org/max-rpm/s1-rpm-subpack-spec-file-changes.html#S3-RPM-SUBPACK-PACKAGE-DIRECTIVE-N-OPTION

* * *

Free-form implementation of

> - comment above "%debug_package" that it is not relevant for
>   Fedora and its removal is pending [there]

per [comment 73] is still missing.

* * *

Digimer, you are also free to cut off the older changelog entries
(like up to and including 1.0-10) to make the situation perhaps a bit
easier to grok, since those steps are all prior to inclusion (and
hence not a game changer for downstream users), and you already
demonstrated your changelog entries are spot-on.  But it's merely
up to you, there was certainly an undeniable effort invested since
the initial version...

(And if you decide to do so, there's no reason to state that very
change in the changelog.)

Oh, and one more nit, %check might have contained, commented out,
something actually usable for said quick check of the basic
functionality.  In case it would be desired in spite of the
mentioned concerns.  This is a mere recommendation.

> Free-form implementation of
> 
>> - comment above "%debug_package" that it is not relevant for
>>   Fedora and its removal is pending [there]
> 
> per [comment 73] is still missing.


Did I misunderstand? I added the comment above "%files -n libknet1-devel";

====
> # libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
> # like other files, is not explicitly versioned in the name like the
> # subpackages are -- intention of doing so for subpackage names is
> # to ease the cross-checking the compatibility of the remote clients
> # interchanging data using this network communication library, as
> # the number denotes the protocol version (providing multiple
> # protocol versions in parallel is not planned).
> %files -n libknet1-devel
====

Anyway, I moved it.

I'm sorry, I don't understand the question/comment/suggestion in comment #77. Can you please restate?

I applied the changes and hit an odd build error against just epel7 on x86_64 pulling in an unexpected RPM on koji. Bumped again from 1.1-5 to -6 to add a version restriction to deal with it. Diff against -4 to -6 below.

New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-6
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-6.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25534544

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25534556

f28:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25534564

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25534572

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25534493


Diff from 1.1-4:
====
--- kronosnet.spec.1.1-4	2018-03-04 02:00:23.290918796 -0500
+++ kronosnet.spec.1.1-6	2018-03-07 01:19:35.321125418 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.1
-Release: 4%{?dist}
+Release: 6%{?dist}
 License: GPLv2+ and LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -93,7 +93,7 @@
 BuildRequires: pkgconfig(zlib)
 %endif
 %if %{defined buildcompresslz4}
-BuildRequires: pkgconfig(liblz4)
+BuildRequires: pkgconfig(liblz4) >= 1.7
 %endif
 %if %{defined buildcompresslzo2}
 BuildRequires: lzo-devel
@@ -257,9 +257,7 @@
 %license COPYING.* COPYRIGHT
 %{_libdir}/libtap.so.*
 
-%ldconfig_scriptlets libtap1
-
-%postun -n libtap1 -p /sbin/ldconfig
+%ldconfig_scriptlets -n libtap1
 
 %package -n libtap1-devel
 Summary: Simple userland wrapper around kernel tap devices (developer files)
@@ -299,9 +297,7 @@
 %{_libdir}/libknet.so.*
 %dir %{_libdir}/kronosnet
 
-%post -n libknet1 -p /sbin/ldconfig
-
-%postun -n libknet1 -p /sbin/ldconfig
+%ldconfig_scriptlets -n libknet1
 
 %package -n libknet1-devel
 Summary: Kronosnet core switching implementation (developer files)
@@ -314,13 +310,6 @@
 Please refer to the not-yet-existing documentation for further
 information. 
 
-# libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
-# like other files, is not explicitly versioned in the name like the
-# subpackages are -- intention of doing so for subpackage names is
-# to ease the cross-checking the compatibility of the remote clients
-# interchanging data using this network communication library, as
-# the number denotes the protocol version (providing multiple
-# protocol versions in parallel is not planned).
 %files -n libknet1-devel
 %{_libdir}/libknet.so
 %{_includedir}/libknet.h
@@ -469,10 +458,25 @@
 %files -n libknet1-plugins-all
 
 %if %{with rpmdebuginfo}
+# libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
+# like other files, is not explicitly versioned in the name like the
+# subpackages are -- intention of doing so for subpackage names is
+# to ease the cross-checking the compatibility of the remote clients
+# interchanging data using this network communication library, as
+# the number denotes the protocol version (providing multiple
+# protocol versions in parallel is not planned).
 %debug_package
 %endif
 
 %changelog
+* Wed Mar 07 2018 Madison Kelly <mkelly> - 1.1-6
+- Added a version requirement to lz4 to deal with koji pulling in the
+  wrong package.
+
+* Tue Mar 06 2018 Madison Kelly <mkelly> - 1.1-5
+- Updated ldconfig scriptlet calls.
+- Moved the debug_package leading comment.
+
 * Sun Mar 04 2018 Madison Kelly <mkelly> - 1.1-4
 - Removed leading spaces from descriptions.
 - Added the (commented out) %%check tests.
@@ -493,45 +497,3 @@
 - Removed the (no longer needed) gcc8-fixes.patch
 - Added the new doxygen and libqb-devel buildrequires for libknetd.
 
-* Fri Feb 23 2018 Madison Kelly <mkelly> - 1.0-10
-- Added missing change log for 1.0-9.
-
-* Thu Feb 22 2018 Madison Kelly <mkelly> - 1.0-9
-- Changed the source tarball to be one from the upstream source.
-- Updated the main license.
-- Moved down the %%{?systemd_requires} macro.
-
-* Fri Feb 16 2018 Madison Kelly <mkelly> - 1.0-8
-- Reverted to 'BuildRequires: bzip2-devel' to fix EPEL7 builds.
-
-* Thu Feb 15 2018 Madison Kelly <mkelly> - 1.0-7
-- Added missing 1.0-6 changelog.
-- Added kronosnetd postun.
-- Clarified licensing.
-- (re)added systemd_requires.
-- Wrapped several buildrequires with pkgconfig().
-
-* Wed Feb 14 2018 Madison Kelly <mkelly> - 1.0-6
-- Removed sysvinit checks.
-- Fixed the groupadd to add to system group.
-- Added build requirement for systemd.
-- Added %%{_isa} macro to knet requirements in sub packages.
-
-* Mon Feb 12 2018 Madison Kelly <mkelly> - 1.0-5
-- All changes related to this spec;
-- Fixed typo in previous changelog date (Tue -> Wed).
-- Capitalized summaries as needed.
-- Changed Requires that were file paths to package names.
-- Added arch and version/release requirements for knet packages.
-- Changed groupadd to be a little smarter.
-
-* Wed Jan 31 2018 Madison Kelly <mkelly> - 1.0-4
-- Added patch for gcc8 fix.
-- Cleaned up description.
-
-* Tue Jan 30 2018 Madison Kelly <mkelly> - 1.0-3
-- Rebuild for Fedora release.
-
-* Tue Jan 30 2018 Autotools generated version <nobody> - 1.0-1-48.d96b.
-- These aren't the droids you're looking for.
-
====

Fixed comments.

As an aside for later; When talking to people on #fedora-devel, it was recommended *not* to use 'BuildRequires: pkgconfig(x)', despite the package guidelines recommending it, as it is more ambiguous. The example given was:

====
issue occurs when various providers exists, ex: pkgconfig(openssl) mays pull openssl-devel or compat-openssl10-devel (the second being a terrible choice)
====

I don't want to hold up the approval process, but I wanted to bring this up as a possible change later. Thoughts?


New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-7
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-7.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25535007

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25535014

f28:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25535022

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25535030

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25535038


Diff from 1.1-6:
====
--- kronosnet.spec.1.1-6	2018-03-07 01:19:35.321125418 -0500
+++ kronosnet.spec.1.1-7	2018-03-07 01:50:40.831722937 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.1
-Release: 6%{?dist}
+Release: 7%{?dist}
 License: GPLv2+ and LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -310,6 +310,13 @@
 Please refer to the not-yet-existing documentation for further
 information. 
 
+# libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
+# like other files, is not explicitly versioned in the name like the
+# subpackages are -- intention of doing so for subpackage names is
+# to ease the cross-checking the compatibility of the remote clients
+# interchanging data using this network communication library, as
+# the number denotes the protocol version (providing multiple
+# protocol versions in parallel is not planned).
 %files -n libknet1-devel
 %{_libdir}/libknet.so
 %{_includedir}/libknet.h
@@ -458,17 +465,15 @@
 %files -n libknet1-plugins-all
 
 %if %{with rpmdebuginfo}
-# libknet.pc leading to pkgconfig(libknet) automatic virtual provides,
-# like other files, is not explicitly versioned in the name like the
-# subpackages are -- intention of doing so for subpackage names is
-# to ease the cross-checking the compatibility of the remote clients
-# interchanging data using this network communication library, as
-# the number denotes the protocol version (providing multiple
-# protocol versions in parallel is not planned).
+# This is left over from upstream.
 %debug_package
 %endif
 
 %changelog
+* Wed Mar 07 2018 Madison Kelly <mkelly> - 1.1-7
+- Moved the comment back above '%%files -n libknet1-devel'.
+- Added comment to '%%debug_package'.
+
 * Wed Mar 07 2018 Madison Kelly <mkelly> - 1.1-6
 - Added a version requirement to lz4 to deal with koji pulling in the
   wrong package.
====

(In reply to digimer from comment #80)

> As an aside for later; When talking to people on #fedora-devel, it was
> recommended *not* to use 'BuildRequires: pkgconfig(x)', despite the package
> guidelines recommending it, as it is more ambiguous. The example given was:

To be clearer, I want to share that pkgconfig(xx) dependency have issues, probably the reason why Guidelines only only states a "SHOULD", and thus xx-devel is a usual workaround (why may have other issues).

Thanks, Remi. 

To add a reference; The rhbz related to the -6 change to 'BuildRequire' is https://bugzilla.redhat.com/show_bug.cgi?id=1552431.

[the review shifts behind my back, not keen on fighting the mills,
I am not an unprofessional rational-processes-bending person, just
my responses and thank you for your work so far]

There's a misunderstanding, "%files -n libknet1-devel" comment should
stay where it was in 1.1.4.

I was asking for a new one to explain the interim character of extra
treatment of debug packages that shouldn't have been introduced in
Fedora context in the first place.

* * *

re [comment 77], I am not familiar with how the test suite is run
for kronosnet, an example command would be "make check".
Nice-to-have category, though, the comment already explains why it
is not so straightforward in this case to run the tests.

* * *

Thanks for dealing with lz4 issues.

Regarding "pkgconfig(openssl)" expression of dependencies, yes, they can
be versioned as well and/or can be combined with "Suggests" to prioritize
particular underlying package name should the conflict on such virtual
provides arise:

https://fedoraproject.org/wiki/Packaging:WeakDependencies#Real_life_example

Depending on how compat packages are structured, the same "satisfied by
more packages" situation could occur also with the previous cryptical
select-by-header-file approach, so there's effectively no regression
in this comparison.

(In reply to Jan Pokorný from comment #83)

> There's a misunderstanding, "%files -n libknet1-devel" comment should
> stay where it was in 1.1.4.
> 
> I was asking for a new one to explain the interim character of extra
> treatment of debug packages that shouldn't have been introduced in
> Fedora context in the first place.

this is already addressed in comment #80

> 
> * * *
> 
> re [comment 77], I am not familiar with how the test suite is run
> for kronosnet, an example command would be "make check".
> Nice-to-have category, though, the comment already explains why it
> is not so straightforward in this case to run the tests.

executing the test is straight forward make check, but we comment it out for safety.

> 
> * * *
> 
> Thanks for dealing with lz4 issues.
> 
> Regarding "pkgconfig(openssl)" expression of dependencies, yes, they can
> be versioned as well and/or can be combined with "Suggests" to prioritize
> particular underlying package name should the conflict on such virtual
> provides arise:
> 
> https://fedoraproject.org/wiki/Packaging:WeakDependencies#Real_life_example
> 
> Depending on how compat packages are structured, the same "satisfied by
> more packages" situation could occur also with the previous cryptical
> select-by-header-file approach, so there's effectively no regression
> in this comparison.

We will just switch back to BuildRequires: package-name.

In context, upstream is also moving away from file based dependencies.

Removed the 'pkgconfig()' method of handling BuildRequires.


New .spec and srpm:
https://www.alteeve.com/an-repo/files/packages/kronosnet.spec.1.1-8
https://www.alteeve.com/an-repo/files/packages/kronosnet-1.1-8.fc27.src.rpm

f26:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25592451

f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25592465

f28:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25592473

rawhide:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25592491

epel7:
https://koji.fedoraproject.org/koji/taskinfo?taskID=25592503


Diff from 1.1-7:
====
--- kronosnet.spec.1.1-7        2018-03-07 01:50:40.831722937 -0500
+++ kronosnet.spec.1.1-8        2018-03-09 19:48:42.630061443 -0500
@@ -70,7 +70,7 @@
 Name: kronosnet
 Summary: Multipoint-to-Multipoint VPN daemon
 Version: 1.1
-Release: 7%{?dist}
+Release: 8%{?dist}
 License: GPLv2+ and LGPLv2+
 URL: http://www.kronosnet.org
 Source0: http://www.kronosnet.org/releases/kronosnet-%{version}.tar.gz
@@ -79,27 +79,27 @@
 BuildRequires: gcc
 # required to build man pages
 BuildRequires: libxml2-devel doxygen
-BuildRequires: pkgconfig(libqb)
+BuildRequires: libqb-devel
 %if %{defined buildsctp}
 BuildRequires: lksctp-tools-devel
 %endif
 %if %{defined buildcryptonss}
-BuildRequires: pkgconfig(nss)
+BuildRequires: nss-devel
 %endif
 %if %{defined buildcryptoopenssl}
-BuildRequires: pkgconfig(openssl)
+BuildRequires: openssl-devel
 %endif
 %if %{defined buildcompresszlib}
-BuildRequires: pkgconfig(zlib)
+BuildRequires: zlib-devel
 %endif
 %if %{defined buildcompresslz4}
-BuildRequires: pkgconfig(liblz4) >= 1.7
+BuildRequires: lz4-devel
 %endif
 %if %{defined buildcompresslzo2}
 BuildRequires: lzo-devel
 %endif
 %if %{defined buildcompresslzma}
-BuildRequires: pkgconfig(liblzma)
+BuildRequires: xz-devel
 %endif
 %if %{defined buildcompressbzip2}
 BuildRequires: bzip2-devel
@@ -470,6 +470,10 @@
 %endif
 
 %changelog
+* Fri Mar 09 2018 Madison Kelly <mkelly> - 1.1-8
+- Changed pkgconfig() to normal package names to help avoid the wrong
+  package being pulled in to satisfy dependencies.
+
 * Wed Mar 07 2018 Madison Kelly <mkelly> - 1.1-7
 - Moved the comment back above '%%files -n libknet1-devel'.
 - Added comment to '%%debug_package'.
====

Package is ok to go into Fedora.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package does not own files or directories owned by other packages.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: kronosnet-debuginfo-1.1-8.fc28.i686.rpm
          kronosnet-debugsource-1.1-8.fc28.i686.rpm
          libknet1-1.1-8.fc28.i686.rpm
          libknet1-devel-1.1-8.fc28.i686.rpm
          libknet1-crypto-nss-plugin-1.1-8.fc28.i686.rpm
          libknet1-crypto-openssl-plugin-1.1-8.fc28.i686.rpm
          libknet1-compress-zlib-plugin-1.1-8.fc28.i686.rpm
          libknet1-compress-lz4-plugin-1.1-8.fc28.i686.rpm
          libknet1-compress-lzo2-plugin-1.1-8.fc28.i686.rpm
          libknet1-compress-lzma-plugin-1.1-8.fc28.i686.rpm
          libknet1-compress-bzip2-plugin-1.1-8.fc28.i686.rpm
          libknet1-crypto-plugins-all-1.1-8.fc28.i686.rpm
          libknet1-compress-plugins-all-1.1-8.fc28.i686.rpm
          libknet1-plugins-all-1.1-8.fc28.i686.rpm
          kronosnet-1.1-8.fc28.src.rpm
kronosnet-debugsource.i686: W: no-documentation
libknet1.i686: W: spelling-error Summary(en_US) Kronosnet -> Kronecker
libknet1.i686: W: spelling-error %description -l en_US Kronosnet -> Kronecker
libknet1.i686: W: spelling-error %description -l en_US knet -> net, knelt, knee
libknet1.i686: W: no-documentation
libknet1.i686: E: library-without-ldconfig-postin /usr/lib/libknet.so.1.1.0
libknet1.i686: E: library-without-ldconfig-postun /usr/lib/libknet.so.1.1.0
libknet1-devel.i686: W: spelling-error Summary(en_US) Kronosnet -> Kronecker
libknet1-devel.i686: W: spelling-error %description -l en_US kronosnet -> Kronecker
libknet1-crypto-nss-plugin.i686: W: no-documentation
libknet1-crypto-openssl-plugin.i686: W: no-documentation
libknet1-compress-zlib-plugin.i686: W: no-documentation
libknet1-compress-lz4-plugin.i686: W: no-documentation
libknet1-compress-lzo2-plugin.i686: W: no-documentation
libknet1-compress-lzma-plugin.i686: W: no-documentation
libknet1-compress-bzip2-plugin.i686: W: no-documentation
libknet1-crypto-plugins-all.i686: W: no-documentation
libknet1-compress-plugins-all.i686: W: no-documentation
libknet1-plugins-all.i686: W: no-documentation
kronosnet.src: W: spelling-error Summary(en_US) Multipoint -> Multipurpose
kronosnet.src:198: W: macro-in-comment %check
15 packages and 0 specfiles checked; 2 errors, 19 warnings.




Rpmlint (debuginfo)
-------------------
Checking: kronosnet-debuginfo-1.1-8.fc28.i686.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

@digimer:
Because fedora-review is now set to +, it should be possible to follow https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner . It basically means to use https://pagure.io/fedrepo_req (or actually fedpkg request-repo) and then just wait till repo is created. It's also good idea to pass all required branches (of course it's possible to do it later).

I've been away for work this last week, but I am returning later today. Finishing this process is my first ToDo.

https://pagure.io/releng/fedora-scm-requests/issue/5210

Note that I had to use the deprecated call (fedrepo-req kronosnet -t 1507103) as this: 

fedpkg request-repo 1507103

Errored with:

Could not execute request_repo: The Bugzilla bug provided is not the proper type

I doubt it makes a difference, but just in case.

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/kronosnet

kronosnet-1.1-8.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-6eae0c4a51

kronosnet-1.1-8.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-94bdc0ccfb

kronosnet-1.1-8.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-603c1c93cf

kronosnet-1.1-8.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-6eae0c4a51

kronosnet-1.1-8.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-603c1c93cf

kronosnet-1.1-8.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

kronosnet-1.4-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-b4cad12543

kronosnet-1.4-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-6280026399

kronosnet-1.4-1.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-b4cad12543

kronosnet-1.4-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-6280026399

kronosnet-1.4-1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.

kronosnet-1.5-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-8e2a9e3dbe

kronosnet-1.5-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-8e2a9e3dbe

kronosnet-1.8-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-4780c5a01a

kronosnet-1.8-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-4780c5a01a

kronosnet-1.8-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.