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

I miss some details :
- I'm the upstream maintainer of Prelude
- I never push package to Fedora but I package prelude for OpenSuse and I help Mageia team to do it also
- I'm looking for a sponsor
- Prelude was packaged in Fedora until F19

Drive-by comments. Not a full review, because due to number of issues, this package will need some more work.

Consider pointing the "fedora-review" tool at this bugzilla ticket. The tool performs test-builds and then runs *many* checks on them, which are relevant to both the package maintainer as well as potential reviewers. Some of the checks, such as the licensing related ones, are extremely helpful.


> %define major                   23
> %define libname                 libprelude%{major}
> %define cppmajor                8
> %define libcpp                  libpreludecpp%{cppmajor}
> %define libnamedevel            libprelude-devel

https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define


> Summary:        Prelude Hybrid Intrusion Detection System Library

The summary could be made even shorter and more concise if not repeating the name of the library here. "Prelude" is an English word with various meanings that adds more ambiguity here than value. The %description expands on the name of this library framework. And there has yet to be a package tool that completely ignores the package %name when displaying a user's search results.

Interestingly, the acronym "IDMEF" and its spelled out words don't appear anywhere in the spec file %summary or %description.


> Group:          System/Libraries

The base group for runtime library packages has been "System Environment/Libraries" for ages.

At Fedora, the Group tag is fully optional for a long time. So long that even the section in the packaging guidelines, which explained that, has been replaced:

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


> Patch0:         libprelude-3.1.0-linking.patch

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


> %define major                   23
> %define libname                 libprelude%{major}
> %package -n %{libname}

Ewwwh. As convenient as such versioned package names may be in some cases, doing it like this is a halfbaked solution. Whenever you would change %major, it would also change package names, and you would end up with orphaned packages in the repositories and on users' installations. Versioned packages make the most sense, if you actually wanted to release multiple parallel-installable APIs/ABIs to choose from. Then you would package older compatibility releases separately and with their own %name.


> %package -n %{libname}
> Provides:       %{name} = %{version}-%{release}

Useless, because incomplete. There are arch-specific automatic Provides these days, and this line would not be sufficient and would break any explicit arch-specific Requires:

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


> Group:          Development/C

This may be a group used by other distributions. It is "Development/Libraries" for Red Hat and Fedora for ages, but the Group tag is optional nowadays anyway. Not going to check all the other group tags.


> Requires:       %{libname} = %{version}-%{release}
> Requires:       %{libcpp} = %{version}-%{release}

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


> %{_libdir}/libprelude.so.%{major}
> %{_libdir}/libprelude.so.%{major}.*

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

Also with regard to the other lib subpackages.


> %files -n %{libnamedevel}
> %doc %{_docdir}/%{libnamedevel}

Anything in and below %_docdir is marked %doc by default (see "rpm -E %__docdir_path").


> %{_datadir}/%{name}/swig

Nothing in the spec file seems to include %_datadir/%name yet.

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> %doc AUTHORS ChangeLog README INSTALL

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


> %attr(0644,root,root) %config(noreplace) %{_sysconfdir}/prelude/default/*.conf

Those are the default attributes for files. Consider adding a comment that explains whether and why you want to override something using %attr here.

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

Thanks for the review, I'll post a new ".spec"

Hello

Here is the new spec : http://pastebin.com/wupPjGUX
And here is the new SRPM : https://copr-be.cloud.fedoraproject.org/results/totol/Prelude/fedora-rawhide-x86_64/00471732-libprelude/libprelude-3.1.0-24.fc26.src.rpm

I still have errors for FSF address not up to date but it is libgpg-error sources. What do I have to do for this ?

Feel free to make other reviews

Thanks

Hello

Can someone help me on this bug ?

I'm also looking for a sponsor

Thanks

Regards

Please don't use pastebin for spec files. It's too ephemeral. Also, link to the "raw" spec file, so that fedora-review and other tools work.

In Fedora, we only package the latest version of any library (in the usual case; it's possible to provide "compat" packages, but this doesn't apply here). So the library subpackage should go away, and the contents should be in the main package. Similarly for the cpp subpackage, it should not have the version in the name.

Please consider reducing the number of macros. They provide no value, and make everything very hard to read. The guidelines say that macros should be used for *directory* names, and using macros for things which cannot ever change is pointless [https://fedoraproject.org/wiki/Packaging:Guidelines#Macros].

You probably don't need to repeat the full %description for libpreludecpp, but you should mention how this subpackage is different from the main subpackage.

%package -n prelude-tools
Summary:        The interface for %{libname}
→
Summary:        Command-line tools for libprelude

python2 subpackage should be called python2-prelude.
%{python_provide} must be used [https://fedoraproject.org/wiki/Packaging:Python#The_.25python_provide_macro].

%setup -q
%autopatch -p1
→ %autosetup -p1

%{_bindir}/chrpath → chrpath (see the link to guidelines for macro usage above).

It is better to avoid using wildcards in the %files section to have an idea what is installed with the package. Doing the following change would make it more safe (same for python3 subpackage):

> %{python2_sitelib}/*

change to

%{python2_sitelib}/whatever
%{python2_sitelib}/whatever-%{version}-py?.?.egg-info

The license files and the basic documentation (AUTHORS ChangeLog README NEWS LICENSE.README HACKING.README) goes in the main subpackage.

The package looks good for such a complicated project. I think you're 90% of the way there ;)

--

I can sponsor you into the packagers group. In addition to the package that you are submitting, I require two-three real reviews of other packages (see http://fedoraproject.org/PackageReviewStatus/NEW.html). There's plenty of python packages awaiting review, so you should be able to find something interesting without any trouble. In your review, please indicate that you are not a packager yet, hence the review is unofficial. If nobody beats you to it, you'll be able to finalize those reviews after you become a packager (hopefully soon ;)). Running fedora-review and carefully looking at the output is a good start.

Oh, it's always good to link to the previous review, if there was one.

Hello

Thanks for the review

Here is the new src file : https://copr-be.cloud.fedoraproject.org/results/totol/Prelude/fedora-rawhide-x86_64/00476797-libprelude/libprelude-3.1.0-24.fc26.src.rpm
And here the new spec file : https://www.prelude-siem.org/pkg/src/3.1.0/libprelude.spec

I have some questions :

- For soname at the end of the subpackage name, this is what other linux distribution required, that's why I do it. It make you able to install multiple libprelude versions in parallel. Do I really have to make only one subpackage with all ".so" files and do not precise the soname in the subpackage name ?

- Can you tell which macro you are talking about ?

- %description is required when you use "%package -n"

OK for evrything else :) See the spec file

--

Thanks for sponsoring :) I will do some reviews this week

(In reply to Thomas Andrejak from comment #9)
> - For soname at the end of the subpackage name, this is what other linux
> distribution required, that's why I do it.
Yes, but Fedora doesn't work this way. In Fedora, there normally just one
version of a given library. Everybody upgrades at once. Hence no suffix.

> It make you able to install
> multiple libprelude versions in parallel. Do I really have to make only one
> subpackage with all ".so" files and do not precise the soname in the
> subpackage name ?
Yes.
 
> - Can you tell which macro you are talking about ?
I think the following macros are unneeded:
%global libname                 libprelude%{major}
%global libcpp                  libpreludecpp%{cppmajor}
%global libnamedevel            libprelude-devel


> - %description is required when you use "%package -n"
OK, let's leave that one for later.

Instead, please explain the difference between package %{libname} and 
package %{libcpp} (see how those macros make everything more obfuscated ;)).
Is there a reason why the second is not merged with the first?

> Thanks for sponsoring :) I will do some reviews this week
Great.

Here is the new version

SRPM : https://copr-be.cloud.fedoraproject.org/results/totol/Prelude/fedora-rawhide-x86_64/00479026-libprelude/libprelude-3.1.0-24.fc26.src.rpm
SPEC : https://www.prelude-siem.org/pkg/src/3.1.0/libprelude.spec

# Force default attrs because libprelude force others
%defattr(- , root, root, 755)
→ I think you don't need this anymore.

%{python3_sitearch}/_prelude.*so
%{python3_sitearch}/prelude.py
→ Not a packaging issue, but still something to reconsider upstream. I think putting a private module at the top level is rather ugly. Imaging the mess if everybody did that ;). Why not structure this as
%{python3_sitearch}/prelude/__init__.py
%{python3_sitearch}/prelude/_prelude.*so
? (Please note that I'm just complaining here, this review is not contingent on this in any way.)

BR: perl-generators is needed according to http://fedoraproject.org/wiki/Packaging:Perl#Build_Dependencies,
and also R: perl(:MODULE_COMPAT), see http://fedoraproject.org/wiki/Packaging:Perl#Versioned_MODULE_COMPAT_Requires.

- Provides: bundled(gnulib) in place as required.
  Note: Bundled gnulib but no Provides: bundled(gnulib)
  See:
  http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Requirement_if_you_bundle

- Note: License file LICENSE.README is not marked as %license
Yeah, it seems reasonable to include that in %license too.
Same goes for HACKING.README.

- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 5703680 bytes in 53 files.
  See:
  http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

I'm not sure how exactly fedora-review arrives at this number, but it seems that there's indeed a few MBs of documentation. You might want to split out libprelude-doc subpackage with /usr/share/doc/libprelude-devel/libprelude.
(Also not that there's an extra level of directories nesting here that looks accidental.)

rpmlint also says:
libprelude-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-to-errno.h
libprelude-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-sources.h
libprelude-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strsource.c
libprelude-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-from-errno.h
libprelude-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-codes.h
libprelude-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strerror.c
It's not a big issue, but probably to fix upstream at some point.

libprelude.x86_64: W: crypto-policy-non-compliance-gnutls-2 /usr/lib64/libprelude.so.23.3.0 gnutls_priority_init
prelude-tools.x86_64: W: crypto-policy-non-compliance-gnutls-1 /usr/bin/prelude-admin gnutls_priority_set_direct

This one is a bigger problem. It's been a while since I looked at the details, but basically you need to call gnutls_set_default_priority or  gnutls_priority_set_direct("@SYSTEM") [https://fedoraproject.org/wiki/Packaging:CryptoPolicies]. If this policy does not apply to this package for some reason, please explain in a comment in the spec file. Looking at ./prelude-admin/server.c, it should be easy enough to patch.

libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libprelude.so.23.3.0 /lib64/libdl.so.2
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libprelude.so.23.3.0 /lib64/libgpg-error.so.0
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libgnutls.so.30
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libgcrypt.so.20
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libdl.so.2
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libgpg-error.so.0
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libltdl.so.7
libprelude.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libm.so.6

Overlinking? Not a big issue, because those libraries are going to be installed anyway, but removing it might reduce memory usage and startup time but some minuscule amount.

And one more suggestion for upstream reconsideration:
custom autoconf macros are horrible. The problem is that any project that wants to use them, must either bundle them (which is annoying if you have more than two or three dependencies), or wrap the calls to those macros in ugly and brittle m4 macros for the case when the dependency is not installed. Please consider providing a pkgconfig file, which is easier to write, easier to use, and as a bonus, works with other build systems like meson. (Please note that I'm just complaining here, this review is not contingent on this in any way.)

(In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> # Force default attrs because libprelude force others
> %defattr(- , root, root, 755)
> → I think you don't need this anymore.

=> Just try it, I still need this

> 
> %{python3_sitearch}/_prelude.*so
> %{python3_sitearch}/prelude.py
> → Not a packaging issue, but still something to reconsider upstream. I think
> putting a private module at the top level is rather ugly. Imaging the mess
> if everybody did that ;). Why not structure this as
> %{python3_sitearch}/prelude/__init__.py
> %{python3_sitearch}/prelude/_prelude.*so
> ? (Please note that I'm just complaining here, this review is not contingent
> on this in any way.)

Good point, I will put this upstream

> 
> BR: perl-generators is needed according to
> http://fedoraproject.org/wiki/Packaging:Perl#Build_Dependencies,
> and also R: perl(:MODULE_COMPAT), see
> http://fedoraproject.org/wiki/Packaging:
> Perl#Versioned_MODULE_COMPAT_Requires.

Done

> 
> - Provides: bundled(gnulib) in place as required.
>   Note: Bundled gnulib but no Provides: bundled(gnulib)
>   See:
>  
> http://fedoraproject.org/wiki/Packaging:
> No_Bundled_Libraries#Requirement_if_you_bundle

Done

> 
> - Note: License file LICENSE.README is not marked as %license
> Yeah, it seems reasonable to include that in %license too.
> Same goes for HACKING.README.

Done

> 
> - Large documentation must go in a -doc subpackage. Large could be size
>   (~1MB) or number of files.
>   Note: Documentation size is 5703680 bytes in 53 files.
>   See:
>   http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation
> 
> I'm not sure how exactly fedora-review arrives at this number, but it seems
> that there's indeed a few MBs of documentation. You might want to split out
> libprelude-doc subpackage with /usr/share/doc/libprelude-devel/libprelude.
> (Also not that there's an extra level of directories nesting here that looks
> accidental.)

Done

> 
> rpmlint also says:
> libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-to-errno.h
> libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-sources.h
> libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strsource.c
> libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-from-errno.h
> libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-codes.h
> libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strerror.c
> It's not a big issue, but probably to fix upstream at some point.

I can't change this, this is not part of libprelude

> 
> libprelude.x86_64: W: crypto-policy-non-compliance-gnutls-2
> /usr/lib64/libprelude.so.23.3.0 gnutls_priority_init
> prelude-tools.x86_64: W: crypto-policy-non-compliance-gnutls-1
> /usr/bin/prelude-admin gnutls_priority_set_direct
> 
> This one is a bigger problem. It's been a while since I looked at the
> details, but basically you need to call gnutls_set_default_priority or 
> gnutls_priority_set_direct("@SYSTEM")
> [https://fedoraproject.org/wiki/Packaging:CryptoPolicies]. If this policy
> does not apply to this package for some reason, please explain in a comment
> in the spec file. Looking at ./prelude-admin/server.c, it should be easy
> enough to patch.

Done. I explain in the spec why it is OK for libprelude.so. For prelude-admin, I made a patch to use what is required in the Wiki but I still have the warning

> 
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libprelude.so.23.3.0 /lib64/libdl.so.2
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libprelude.so.23.3.0 /lib64/libgpg-error.so.0
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libgnutls.so.30
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libgcrypt.so.20
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libdl.so.2
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libgpg-error.so.0
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libltdl.so.7
> libprelude.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libpreludecpp.so.8.1.0 /lib64/libm.so.6
> 
> Overlinking? Not a big issue, because those libraries are going to be
> installed anyway, but removing it might reduce memory usage and startup time
> but some minuscule amount.

Done

> 
> And one more suggestion for upstream reconsideration:
> custom autoconf macros are horrible. The problem is that any project that
> wants to use them, must either bundle them (which is annoying if you have
> more than two or three dependencies), or wrap the calls to those macros in
> ugly and brittle m4 macros for the case when the dependency is not
> installed. Please consider providing a pkgconfig file, which is easier to
> write, easier to use, and as a bonus, works with other build systems like
> meson. (Please note that I'm just complaining here, this review is not
> contingent on this in any way.)

Interesting, I will share this with upstream

Here are the new files

spec : https://www.prelude-siem.org/pkg/src/3.1.0/libprelude.spec

srpm : https://copr-be.cloud.fedoraproject.org/results/totol/Prelude/fedora-rawhide-x86_64/00481802-libprelude/libprelude-3.1.0-26.fc26.src.rpm

(In reply to Thomas Andrejak from comment #13)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> > libprelude.x86_64: W: crypto-policy-non-compliance-gnutls-2
> > /usr/lib64/libprelude.so.23.3.0 gnutls_priority_init
> > prelude-tools.x86_64: W: crypto-policy-non-compliance-gnutls-1
> > /usr/bin/prelude-admin gnutls_priority_set_direct
> > 
> > This one is a bigger problem. It's been a while since I looked at the
> > details, but basically you need to call gnutls_set_default_priority or 
> > gnutls_priority_set_direct("@SYSTEM")
> > [https://fedoraproject.org/wiki/Packaging:CryptoPolicies]. If this policy
> > does not apply to this package for some reason, please explain in a comment
> > in the spec file. Looking at ./prelude-admin/server.c, it should be easy
> > enough to patch.
> 
> Done. I explain in the spec why it is OK for libprelude.so. For
> prelude-admin, I made a patch to use what is required in the Wiki but I
> still have the warning

> # - crypto-policy-non-compliance-gnutls-2 in libprelude.so is normal
> #   because the user can configure it

That's the only important issue. While it is true that the value
can be overridden, most users of the library will not override the
default, so the "default" provided by the library is what will be
used most often in the end. IIUC, libprelude.so has
tls_auth_init_priority() which calls gnutls_priority_init(tlsopts ?: "NORMAL"),
and tls_auth_init_priority() is called in a few places as 
tls_auth_init_priority(NULL), e.g. from tls_auth_connection().
So effectively "NORMAL" is the default. I think it should be changed
to "@SYSTEM".

For a similar case, consider libmicrohttpd. We patched it
[http://pkgs.fedoraproject.org/cgit/rpms/libmicrohttpd.git/tree/gnutls-utilize-system-crypto-policy.patch]
to use @SYSTEM, and all users of libmicrohttpd magically became
compliant with the policy without any further work.

> > # Force default attrs because libprelude force others
> > %defattr(- , root, root, 755)
> > → I think you don't need this anymore.
> 
> => Just try it, I still need this

> > - Provides: bundled(gnulib) in place as required.
> >   Note: Bundled gnulib but no Provides: bundled(gnulib)
> >   See:
> >  
> > http://fedoraproject.org/wiki/Packaging:
> > No_Bundled_Libraries#Requirement_if_you_bundle
> 
> Done

Note: it'd be nicer to include a specific "version", something like
Provides: bundled(gnulib) = 20160508
but if it's hard to determine or unclear or whatever, it's fine as is.

> > rpmlint also says:
> > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-to-errno.h
> > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-sources.h
> > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strsource.c
> > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-from-errno.h
> > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-codes.h
> > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strerror.c
> > It's not a big issue, but probably to fix upstream at some point.
> 
> I can't change this, this is not part of libprelude

Those same files in libgpg-error sources are quite different. It seems that
libprelude has an independent forked copy, so you *could* fix the address.
But anyway, it doesn't matter much.

I noticed another issue: valgrind is used in tests, but is not listed in
BuildRequires. Unfortunately, valgrind is not available on all architectures.
There's a macro %{valgrind_arches}, but it is only available in rawhide.
That could be used in the future, but for now:
%ifnarch s390
BuildRequires: valgrind
%endif
... and you also have to make sure that the test still run without valgrind.

>> And one more suggestion for upstream reconsideration:
>> custom autoconf macros are horrible.
> Interesting, I will share this with upstream
Oh, I now see that there's a pkg-config file already. So my rant wasn't
necessary. Sorry.

Scratch build in koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=16660708
(koji build  --scratch rawhide libprelude-3.1.0-26.fc26.src.rpm)

Tests fail on ppc64 ;(

New spec : https://www.prelude-siem.org/pkg/src/3.1.0/libprelude.spec
New srpm : https://copr-be.cloud.fedoraproject.org/results/totol/Prelude/fedora-26-x86_64/00483991-libprelude/libprelude-3.1.0-26.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #15)
> (In reply to Thomas Andrejak from comment #13)
> > (In reply to Zbigniew Jędrzejewski-Szmek from comment #12)
> > > libprelude.x86_64: W: crypto-policy-non-compliance-gnutls-2
> > > /usr/lib64/libprelude.so.23.3.0 gnutls_priority_init
> > > prelude-tools.x86_64: W: crypto-policy-non-compliance-gnutls-1
> > > /usr/bin/prelude-admin gnutls_priority_set_direct
> > > 
> > > This one is a bigger problem. It's been a while since I looked at the
> > > details, but basically you need to call gnutls_set_default_priority or 
> > > gnutls_priority_set_direct("@SYSTEM")
> > > [https://fedoraproject.org/wiki/Packaging:CryptoPolicies]. If this policy
> > > does not apply to this package for some reason, please explain in a comment
> > > in the spec file. Looking at ./prelude-admin/server.c, it should be easy
> > > enough to patch.
> > 
> > Done. I explain in the spec why it is OK for libprelude.so. For
> > prelude-admin, I made a patch to use what is required in the Wiki but I
> > still have the warning
> 
> > # - crypto-policy-non-compliance-gnutls-2 in libprelude.so is normal
> > #   because the user can configure it
> 
> That's the only important issue. While it is true that the value
> can be overridden, most users of the library will not override the
> default, so the "default" provided by the library is what will be
> used most often in the end. IIUC, libprelude.so has
> tls_auth_init_priority() which calls gnutls_priority_init(tlsopts ?:
> "NORMAL"),
> and tls_auth_init_priority() is called in a few places as 
> tls_auth_init_priority(NULL), e.g. from tls_auth_connection().
> So effectively "NORMAL" is the default. I think it should be changed
> to "@SYSTEM".
> 
> For a similar case, consider libmicrohttpd. We patched it
> [http://pkgs.fedoraproject.org/cgit/rpms/libmicrohttpd.git/tree/gnutls-
> utilize-system-crypto-policy.patch]
> to use @SYSTEM, and all users of libmicrohttpd magically became
> compliant with the policy without any further work.

I fixed it but I still get the warning, dunno why ...

> 
> > > # Force default attrs because libprelude force others
> > > %defattr(- , root, root, 755)
> > > → I think you don't need this anymore.
> > 
> > => Just try it, I still need this
> 
> > > - Provides: bundled(gnulib) in place as required.
> > >   Note: Bundled gnulib but no Provides: bundled(gnulib)
> > >   See:
> > >  
> > > http://fedoraproject.org/wiki/Packaging:
> > > No_Bundled_Libraries#Requirement_if_you_bundle
> > 
> > Done
> 
> Note: it'd be nicer to include a specific "version", something like
> Provides: bundled(gnulib) = 20160508
> but if it's hard to determine or unclear or whatever, it's fine as is.

Done

> 
> > > rpmlint also says:
> > > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-to-errno.h
> > > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-sources.h
> > > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strsource.c
> > > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/code-from-errno.h
> > > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/err-codes.h
> > > libprelude-debuginfo.x86_64: E: incorrect-fsf-address
> > > /usr/src/debug/libprelude-3.1.0/src/libprelude-error/strerror.c
> > > It's not a big issue, but probably to fix upstream at some point.
> > 
> > I can't change this, this is not part of libprelude
> 
> Those same files in libgpg-error sources are quite different. It seems that
> libprelude has an independent forked copy, so you *could* fix the address.
> But anyway, it doesn't matter much.

Done

> 
> I noticed another issue: valgrind is used in tests, but is not listed in
> BuildRequires. Unfortunately, valgrind is not available on all architectures.
> There's a macro %{valgrind_arches}, but it is only available in rawhide.
> That could be used in the future, but for now:
> %ifnarch s390
> BuildRequires: valgrind
> %endif
> ... and you also have to make sure that the test still run without valgrind.

I have to check this but it should works

> 
> >> And one more suggestion for upstream reconsideration:
> >> custom autoconf macros are horrible.
> > Interesting, I will share this with upstream
> Oh, I now see that there's a pkg-config file already. So my rant wasn't
> necessary. Sorry.

Don't worry ;)

> Scratch build in koji: http://koji.fedoraproject.org 
> /koji/taskinfo?taskID=16660708
> (koji build  --scratch rawhide libprelude-3.1.0-26.fc26.src.rpm)

It works when I tried, can you try again ?

*** Bug 1380180 has been marked as a duplicate of this bug. ***

fedora-review says:

- Package does not contain duplicates in %files.
  Note: warning: File listed twice:
  /usr/lib64/perl5/vendor_perl/auto/Prelude/Prelude.so
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

(Use "%dir %{perl_vendorarch}/auto/Prelude")

fedora-review has some other complaints, but they are invalid afaict.

Rpmlint
-------
Checking: libprelude-3.1.0-26.fc26.x86_64.rpm
          libprelude-devel-3.1.0-26.fc26.x86_64.rpm
          prelude-tools-3.1.0-26.fc26.x86_64.rpm
          python2-prelude-3.1.0-26.fc26.x86_64.rpm
          python3-prelude-3.1.0-26.fc26.x86_64.rpm
          perl-prelude-3.1.0-26.fc26.x86_64.rpm
          ruby-prelude-3.1.0-26.fc26.x86_64.rpm
          lua-prelude-3.1.0-26.fc26.x86_64.rpm
          libprelude-doc-3.1.0-26.fc26.noarch.rpm
          libprelude-debuginfo-3.1.0-26.fc26.x86_64.rpm
          libprelude-3.1.0-26.fc26.src.rpm
libprelude.x86_64: W: crypto-policy-non-compliance-gnutls-2 /usr/lib64/libprelude.so.23.3.0 gnutls_priority_init
prelude-tools.x86_64: W: crypto-policy-non-compliance-gnutls-1 /usr/bin/prelude-admin gnutls_priority_set_direct
This is probably a bug in the rpmlint checker.

libprelude-devel.x86_64: W: only-non-binary-in-usr-lib
prelude-tools.x86_64: W: spelling-error Summary(en_US) libprelude -> lib prelude, lib-prelude, prelude
prelude-tools.x86_64: W: no-manual-page-for-binary prelude-adduser
python2-prelude.x86_64: W: no-documentation
python3-prelude.x86_64: W: no-documentation
perl-prelude.x86_64: W: no-documentation
ruby-prelude.x86_64: W: no-documentation
lua-prelude.x86_64: W: no-documentation
libprelude-doc.noarch: W: spelling-error %description -l en_US gtk -> gt, gt k
11 packages and 0 specfiles checked; 0 errors, 11 warnings.

All OK.

+ package name is OK
+ license is ACCEPTABLE (various variants of GPL)
- license is specified correctly (see below)
+ builds and installs OK
+ P/R/BR look correct
+ scriptlets look OK
+ %check is present and passes

The license tag is not correct though: the license tag applies to the *binary* rpms [https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#Does_the_License:_tag_cover_the_SRPM_or_the_binary_RPM.3F, https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F].
In this case:
# libmissing is LGPL-2.1+ → built as part of the final product, compatible with GPL
# libmissing/test is GPL-3.0+ → can be ignored, because this is not distributed as part of the binary rpm
# Prelude is GPL-2.0+ → This is the effective license

Build fails in koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=16709324, on arm64 this time.
The error doesn't look to bad, but it's most likely an upstream issue,
so I think it's fine if you use ExcludeArch until it's fixed.

prelude-log.c: In function 'do_log_v':
prelude-log.c:51:50: error: incompatible type for argument 1 of 'memmove'
 #  define PRELUDE_VA_COPY(ap1, ap2)     memmove ((ap1), (ap2), sizeof(va_list))
                                                  ^
prelude-log.c:229:9: note: in expansion of macro 'PRELUDE_VA_COPY'
         PRELUDE_VA_COPY(bkp, ap);
         ^~~~~~~~~~~~~~~
In file included from /usr/include/features.h:397:0,
                 from /usr/include/sys/types.h:25,
                 from ../libmissing/sys/types.h:28,
                 from ../libmissing/ftw_.h:20,
                 from ./include/libmissing.h:34,
                 from prelude-log.c:24:
/usr/include/bits/string3.h:57:1: note: expected 'void *' but argument is of type 'va_list {aka __va_list}'
 __NTH (memmove (void *__dest, const void *__src, size_t __len))
 ^
prelude-log.c:51:57: error: incompatible type for argument 2 of 'memmove'
 #  define PRELUDE_VA_COPY(ap1, ap2)     memmove ((ap1), (ap2), sizeof(va_list))
                                                         ^
prelude-log.c:229:9: note: in expansion of macro 'PRELUDE_VA_COPY'
         PRELUDE_VA_COPY(bkp, ap);
         ^~~~~~~~~~~~~~~
In file included from /usr/include/features.h:397:0,
                 from /usr/include/sys/types.h:25,
                 from ../libmissing/sys/types.h:28,
                 from ../libmissing/ftw_.h:20,
                 from ./include/libmissing.h:34,
                 from prelude-log.c:24:
/usr/include/bits/string3.h:57:1: note: expected 'const void *' but argument is of type 'va_list {aka __va_list}'
 __NTH (memmove (void *__dest, const void *__src, size_t __len))
 ^

OK, so we have two simple issues (repeated file, license), and the failing builds
on more exotic architectures. You said it built for you, but did you use the
rawhide koji which has more architectures enabled? The failure looks deterministic.
Looks very nice otherwise.

Package is APPROVED. Please fix up the remaining issues when uploading.

--

I've added you to the packagers group. Have fun, use your powers for good, etc ;)
If you have any questions I'm always happy to help (the bugzilla e-mail or zbyszek
on #fedora-devel).

libprelude-3.1.0-26.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-6e12f43ea4

libprelude-3.1.0-26.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-3fe04fb903

libprelude-3.1.0-26.fc25 has been pushed to the Fedora 25 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-2016-6e12f43ea4

libprelude-3.1.0-26.fc24 has been pushed to the Fedora 24 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-2016-3fe04fb903

libprelude-3.1.0-26.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

libprelude-3.1.0-26.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

libprelude-3.1.0-26.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-0ccc77adf2

libprelude-3.1.0-26.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-2016-0ccc77adf2

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