Bugzilla (bugzilla.redhat.com) will be under maintenance for infrastructure upgrades and will not be unavailable on July 31st between 12:30 AM - 05:30 AM UTC. We appreciate your understanding and patience. You can follow status.redhat.com for details.
Bug 1412128 - Review Request: libpreludedb - Prelude DB Library
Summary: Review Request: libpreludedb - Prelude DB Library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1386938
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-11 10:19 UTC by Thomas Andrejak
Modified: 2017-02-05 03:19 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-01-23 00:50:57 UTC
Type: Bug
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Thomas Andrejak 2017-01-11 10:19:38 UTC
This is the continuation of #1386938 . This ticket is for libpreludedb, the library to manage the IDMEF database of Prelude.

Description in spec :
The PreludeDB Library provides an abstraction layer upon the type
and the format of the database used to store IDMEF alerts. It
allows developers to use the Prelude IDMEF database easily and
efficiently without worrying about SQL, and to access the
database independently of the type/format of the database.

SPEC : https://www.prelude-siem.org/pkg/src/3.1.0/libpreludedb.spec
SRPM : https://www.prelude-siem.org/pkg/src/3.1.0/libpreludedb-3.1.0-26.fc26.src.rpm
Build : https://koji.fedoraproject.org/koji/taskinfo?taskID=17242170

Thanks for the review

Regards

Thomas

Comment 1 Zbigniew Jędrzejewski-Szmek 2017-01-11 16:27:45 UTC
It'd be nice to rewrap the %description's. They are very unevenly indented.

> Provides:       %{name}%{?_isa}-devel = %{version}-%{release}
> Provides:       prelude-devel = %{version}-%{release}
That's a bit unusual. Automatically generated provides are usually enough. Why is this needed?

make distcheck → %make_build distcheck. This will add -j..., which should make the whole thing faster. Also, I'm not sure if running "distcheck" is appropriate. "check" should be enough. "distcheck" is for tarball creators to make sure that all the necessary files are included in the distribution, and takes much more time because of the additional rebuild.

There seems to be some codepage corruption in changelog: Caolán McNamara → Caolán McNamara, Marcela MaÅ¡láňová → Marcela Mašláňová.

Looks great though.

Comment 2 Zbigniew Jędrzejewski-Szmek 2017-01-11 16:59:52 UTC
fedora-review says:

- Package does not contain duplicates in %files.
  Note: warning: File listed twice:
  /usr/share/libpreludedb/classic/mysql2pgsql.sh
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

[ ]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define major 7, %define
     cppmajor 2

libpreludedb.i686: W: shared-lib-calls-exit /usr/lib/libpreludedb.so.7.1.1 exit@GLIBC_2.0
Might be worth checking, although usually this is a false positive.

python3-preludedb.i686: E: python-bytecode-wrong-magic-value /usr/lib/python3.6/site-packages/__pycache__/preludedb.cpython-36.pyc expected 3361 (3.6), found 3379 (unknown)
python3-preludedb.i686: E: python-bytecode-wrong-magic-value /usr/lib/python3.6/site-packages/__pycache__/preludedb.cpython-36.opt-1.pyc expected 3361 (3.6), found 3379 (unknown)

I think this is a false positive in rpmlint... python3 seems to import those files just fine.


python2-preludedb needs Requires: python2-prelude,
python3-preludedb needs Requires: python3-prelude.

Comment 3 Thomas Andrejak 2017-01-11 22:33:42 UTC
SPEC : https://fedorapeople.org/~totol/libpreludedb.spec
SRPM : https://fedorapeople.org/~totol/libpreludedb-3.1.0-26.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> It'd be nice to rewrap the %description's. They are very unevenly indented.
> 

Done

> > Provides:       %{name}%{?_isa}-devel = %{version}-%{release}
> > Provides:       prelude-devel = %{version}-%{release}
> That's a bit unusual. Automatically generated provides are usually enough.
> Why is this needed?

There is a mistake, it should be preludedb-devel. This was an habit, as I do in libprelude. But this is just for the lib. Do I have to remove it ?

> 
> make distcheck → %make_build distcheck. This will add -j..., which should
> make the whole thing faster. Also, I'm not sure if running "distcheck" is
> appropriate. "check" should be enough. "distcheck" is for tarball creators
> to make sure that all the necessary files are included in the distribution,
> and takes much more time because of the additional rebuild.

Done

> 
> There seems to be some codepage corruption in changelog: Caolán McNamara →
> Caolán McNamara, Marcela MaÅ¡láňová → Marcela Mašláňová.
> 

Done

> Looks great though.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #2)
> fedora-review says:
> 
> - Package does not contain duplicates in %files.
>   Note: warning: File listed twice:
>   /usr/share/libpreludedb/classic/mysql2pgsql.sh
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
> 

I do not have this on my side, can you help me on this ?

> [ ]: Spec use %global instead of %define unless justified.
>      Note: %define requiring justification: %define major 7, %define
>      cppmajor 2

Done

> 
> libpreludedb.i686: W: shared-lib-calls-exit /usr/lib/libpreludedb.so.7.1.1
> exit@GLIBC_2.0
> Might be worth checking, although usually this is a false positive.

As for libprelude, this is a false positive

> 
> python3-preludedb.i686: E: python-bytecode-wrong-magic-value
> /usr/lib/python3.6/site-packages/__pycache__/preludedb.cpython-36.pyc
> expected 3361 (3.6), found 3379 (unknown)
> python3-preludedb.i686: E: python-bytecode-wrong-magic-value
> /usr/lib/python3.6/site-packages/__pycache__/preludedb.cpython-36.opt-1.pyc
> expected 3361 (3.6), found 3379 (unknown)
> 
> I think this is a false positive in rpmlint... python3 seems to import those
> files just fine.

Yes : https://bugzilla.redhat.com/show_bug.cgi?id=1409376

> 
> 
> python2-preludedb needs Requires: python2-prelude,
> python3-preludedb needs Requires: python3-prelude.

Done

Comment 4 Zbigniew Jędrzejewski-Szmek 2017-01-12 15:47:06 UTC
(In reply to Thomas Andrejak from comment #3)
> > > Provides:       %{name}%{?_isa}-devel = %{version}-%{release}
> > > Provides:       prelude-devel = %{version}-%{release}
> > That's a bit unusual. Automatically generated provides are usually enough.
> > Why is this needed?
> 
> There is a mistake, it should be preludedb-devel. This was an habit, as I do
> in libprelude. But this is just for the lib. Do I have to remove it ?

Oh, I didn't even see that it's "prelude" not "preludedb".
But even with that correction, it still doesn't make sense:
rpm will automatically generate the following:
Provides: prelude-devel = %{version}-%{release}
Provides: prelude-devel%{?_isa} = %{version}-%{release}

and the spec file has:
Provides:       %{name}%{?_isa}-devel = %{version}-%{release}
Provides:       preludedb-devel = %{version}-%{release}

The second of those is redundant, and the first is strange because
it has %{_isa} in the wrong place. You don't _have_ to remove it,
but since it doesn't seem to serve any purpose, you _should_.
It seem to be some legacy craft, and since you're unretiring the
package, you can start with a clean slate.

> > - Package does not contain duplicates in %files.
> >   Note: warning: File listed twice:
> >   /usr/share/libpreludedb/classic/mysql2pgsql.sh
> >   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
> 
> I do not have this on my side, can you help me on this ?

%files devel
%{_datadir}/%{name}
...
%attr(0755,-,-) %{_datadir}/%{name}/classic/mysql2pgsql.sh

So that file is listed twice, in the sense that it's included recursively
in the directory, and later separately.

The best option imho would be to just drop mysql2pgsql.sh, mysql2sqlite.sh
from the %files section completely. Looking into the tarball, they seem
to have proper permissions already.

Comment 5 Zbigniew Jędrzejewski-Szmek 2017-01-12 16:00:24 UTC
Summary right now basically repeats the name. Something like "database to store IDMEF alerts" would be better.

Comment 6 Thomas Andrejak 2017-01-12 21:59:26 UTC
SPEC : https://fedorapeople.org/~totol/libpreludedb.spec
SRPM : https://fedorapeople.org/~totol/libpreludedb-3.1.0-26.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> Summary right now basically repeats the name. Something like "database to
> store IDMEF alerts" would be better.

Done

(In reply to Zbigniew Jędrzejewski-Szmek from comment #4)
> (In reply to Thomas Andrejak from comment #3)
> > > > Provides:       %{name}%{?_isa}-devel = %{version}-%{release}
> > > > Provides:       prelude-devel = %{version}-%{release}
> > > That's a bit unusual. Automatically generated provides are usually enough.
> > > Why is this needed?
> > 
> > There is a mistake, it should be preludedb-devel. This was an habit, as I do
> > in libprelude. But this is just for the lib. Do I have to remove it ?
> 
> Oh, I didn't even see that it's "prelude" not "preludedb".
> But even with that correction, it still doesn't make sense:
> rpm will automatically generate the following:
> Provides: prelude-devel = %{version}-%{release}
> Provides: prelude-devel%{?_isa} = %{version}-%{release}
> 
> and the spec file has:
> Provides:       %{name}%{?_isa}-devel = %{version}-%{release}
> Provides:       preludedb-devel = %{version}-%{release}
> 
> The second of those is redundant, and the first is strange because
> it has %{_isa} in the wrong place. You don't _have_ to remove it,
> but since it doesn't seem to serve any purpose, you _should_.
> It seem to be some legacy craft, and since you're unretiring the
> package, you can start with a clean slate.

You are right, I did it

> 
> > > - Package does not contain duplicates in %files.
> > >   Note: warning: File listed twice:
> > >   /usr/share/libpreludedb/classic/mysql2pgsql.sh
> > >   See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
> > 
> > I do not have this on my side, can you help me on this ?
> 
> %files devel
> %{_datadir}/%{name}
> ...
> %attr(0755,-,-) %{_datadir}/%{name}/classic/mysql2pgsql.sh
> 
> So that file is listed twice, in the sense that it's included recursively
> in the directory, and later separately.
> 
> The best option imho would be to just drop mysql2pgsql.sh, mysql2sqlite.sh
> from the %files section completely. Looking into the tarball, they seem
> to have proper permissions already.

There is an issues with permissions. That was why I put it in this way the first time. I move the chmod into the install so the %files can be as it must be.



Note: I will update the libprelude package with each part of the actual review that we forgot in the libprelude review.

Comment 7 Zbigniew Jędrzejewski-Szmek 2017-01-12 22:37:50 UTC
Hm, I still see %define, not %global. Please fix that up when importing.

> Group:          System Environment/Libraries
Not necessary [https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections]

> Release:        26%{?dist}
Why so high? You could reset it to 1 when importing.

+ package name is OK
+ license is acceptable (GPLv2+)
+ license is specified correctly
+ builds and installs fine
+ provides/requires/buildrequires look sane
+ scriptlets are OK
+ %check is present and passes
+ python packaging guidelines are used
+ %python_provide are present

I think you could add
Suggests: preludedb-mysql
Suggests: preludedb-pgsql
Suggests: preludedb-sqlite3
in the main package.

Package is APPROVED.

Comment 8 Thomas Andrejak 2017-01-13 06:19:49 UTC
SPEC : https://fedorapeople.org/~totol/libpreludedb.spec
SRPM : https://fedorapeople.org/~totol/libpreludedb-3.1.0-26.fc26.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> Hm, I still see %define, not %global. Please fix that up when importing.
> 

done

> > Group:          System Environment/Libraries
> Not necessary
> [https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections]
> 

done

> > Release:        26%{?dist}
> Why so high? You could reset it to 1 when importing.
> 

done

> + package name is OK
> + license is acceptable (GPLv2+)
> + license is specified correctly
> + builds and installs fine
> + provides/requires/buildrequires look sane
> + scriptlets are OK
> + %check is present and passes
> + python packaging guidelines are used
> + %python_provide are present
> 
> I think you could add
> Suggests: preludedb-mysql
> Suggests: preludedb-pgsql
> Suggests: preludedb-sqlite3
> in the main package.

done

> 
> Package is APPROVED.

Thanks for the review !

Comment 9 Fedora Update System 2017-01-14 23:35:18 UTC
libpreludedb-3.1.0-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-a0711d51ee

Comment 10 Fedora Update System 2017-01-15 11:20:38 UTC
libpreludedb-3.1.0-1.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-2017-a0711d51ee

Comment 11 Fedora Update System 2017-01-17 00:20:21 UTC
libpreludedb-3.1.0-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-7751501934

Comment 12 Fedora Update System 2017-01-17 21:50:43 UTC
libpreludedb-3.1.0-1.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-2017-7751501934

Comment 13 Fedora Update System 2017-01-19 06:51:46 UTC
libpreludedb-3.1.0-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-033c0d1778

Comment 14 Fedora Update System 2017-01-20 18:47:07 UTC
libpreludedb-3.1.0-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-2017-033c0d1778

Comment 15 Fedora Update System 2017-01-23 00:50:57 UTC
libpreludedb-3.1.0-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2017-01-25 19:48:57 UTC
libpreludedb-3.1.0-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2017-02-05 03:19:44 UTC
libpreludedb-3.1.0-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.


Note You need to log in before you can comment on or make changes to this bug.