[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #17 from Fedora Update System  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #16 from Fedora Update System  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |CLOSED
 Resolution|--- |ERRATA
Last Closed||2017-01-22 19:50:57



--- Comment #15 from Fedora Update System  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #14 from Fedora Update System  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #13 from Fedora Update System  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |MODIFIED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #12 from Fedora Update System  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ON_QA   |MODIFIED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #11 from Fedora Update System  ---
libpreludedb-3.1.0-1.fc25 has been submitted as an update to Fedora 25.
https://bodhi.fedoraproject.org/updates/FEDORA-2017-7751501934

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|MODIFIED|ON_QA



--- Comment #10 from Fedora Update System  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Fedora Update System  changed:

   What|Removed |Added

 Status|ASSIGNED|MODIFIED



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #9 from Fedora Update System  ---
libpreludedb-3.1.0-1.fc24 has been submitted as an update to Fedora 24.
https://bodhi.fedoraproject.org/updates/FEDORA-2017-a0711d51ee

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #8 from Thomas Andrejak  ---
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 !

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #7 from Zbigniew Jędrzejewski-Szmek  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #6 from Thomas Andrejak  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #4 from Zbigniew Jędrzejewski-Szmek  ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #3 from Thomas Andrejak  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Assignee|nob...@fedoraproject.org|zbys...@in.waw.pl
  Flags||fedora-review?



--- Comment #2 from Zbigniew Jędrzejewski-Szmek  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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



--- Comment #1 from Zbigniew Jędrzejewski-Szmek  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1412128] Review Request: libpreludedb - Prelude DB Library

[bugzilla]

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

Zbigniew Jędrzejewski-Szmek  changed:

   What|Removed |Added

 CC||zbys...@in.waw.pl
 Depends On||1386938




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1386938
[Bug 1386938] Review Request: libprelude - Prelude Library
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org