https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Bug ID: 1352408 Summary: Review Request: lasem - A library for rendering SVG and Mathml, implementing a DOM like API Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: belegdol@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://belegdol.fedorapeople.org/lasem/lasem.spec SRPM URL: https://belegdol.fedorapeople.org/lasem/lasem-0.4.3-1.fc22.src.rpm Description: Lasem is a library for rendering SVG and Mathml, implementing a DOM like API. It's based on GObject and use Pango and Cairo for the rendering. Fedora Account System Username: belegdol
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Igor Gnatenko ignatenko@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |ignatenko@redhat.com Assignee|nobody@fedoraproject.org |ignatenko@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Igor Gnatenko ignatenko@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias| |lasem
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #3 from Igor Gnatenko ignatenko@redhat.com --- (In reply to Julian Sikorski from comment #2)
(In reply to Igor Gnatenko from comment #1) Thank you for taking the review! Please find my feedback below:
Missing BuildRequires: gcc
Has something changed? I thought gcc was part of minimal buildroot and does not need to be required explicitly. In any case, something is pulling it in as the build is working.
Something changed. Since (~ f23) it's required to put all BRs like gcc.
BuildRequires: intltool
I would add also BuildRequires: gettext
Intltool pulls gettext by requiring gettext-devel, is an explicit requirement necessary?
No, hopefully at some point upstream will stop using intltool and we will switch to gettext.
Requires: pkg-config
Drop it
OK
make %{?_smp_mflags}
could be replaced with %make_build
OK, it was not in the template created by rpmdev-newspec on f22.
rpmdev-newspec is completely outdated.
rm -rf $RPM_BUILD_ROOT
Drop it
Why? rpmdev-newspec still put it in as of f24
It's not needed since ~ EL6.
find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';'
find %{buildroot}%{_libdir} -type f -name '*.la' -delete -print
Why is this needed? The version I have now is what rpmdev-newspec as of f24 enters.
Not all packages maintained well. When you use -delete, it doesn't spawn any command, it just uses unlinkat().
%find_lang %{name} --all-name
I think --all-name is not needed, but not sure.
I replaced it with %{name}-%{apiver}
%{_datadir}/gtk-doc/html/%{name}-0.4
%doc %{_datadir}/gtk-doc/html/%{name}-0.4
OK
%dir %{_datadir}/gtk-doc %dir %{_datadir}/gtk-doc/html
you don't need to do this.
$RPM_BUILD_ROOT
%{buildroot}
I prefer the variable format and as per current guidelines [1] both are acceptable.
hopefully it will be deprecated at some point. it's better to use %{buildroot}. but as you noted, it's up to you.
also would be great to do: %global apiver 0.4 and use it in %files.
OK
New releases: Spec URL: https://belegdol.fedorapeople.org/lasem/lasem.spec SRPM URL: https://belegdol.fedorapeople.org/lasem/lasem-0.4.3-2.fc24.src.rpm
[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot. 7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
Resolution: ALMOST GOOD
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #4 from Julian Sikorski belegdol@gmail.com --- (In reply to Igor Gnatenko from comment #3)
(In reply to Julian Sikorski from comment #2)
(In reply to Igor Gnatenko from comment #1) Thank you for taking the review! Please find my feedback below:
Missing BuildRequires: gcc
Has something changed? I thought gcc was part of minimal buildroot and does not need to be required explicitly. In any case, something is pulling it in as the build is working.
Something changed. Since (~ f23) it's required to put all BRs like gcc.
Thank you! Will add it.
BuildRequires: intltool
I would add also BuildRequires: gettext
Intltool pulls gettext by requiring gettext-devel, is an explicit requirement necessary?
No, hopefully at some point upstream will stop using intltool and we will switch to gettext.
If/when it happens BR adjustment will be necessary anyway, so I prefer to keep things as they are.
Requires: pkg-config
Drop it
OK
make %{?_smp_mflags}
could be replaced with %make_build
OK, it was not in the template created by rpmdev-newspec on f22.
rpmdev-newspec is completely outdated.
rm -rf $RPM_BUILD_ROOT
Drop it
Why? rpmdev-newspec still put it in as of f24
It's not needed since ~ EL6.
Didn't know that, thank you! Will change it.
find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';'
find %{buildroot}%{_libdir} -type f -name '*.la' -delete -print
Why is this needed? The version I have now is what rpmdev-newspec as of f24 enters.
Not all packages maintained well. When you use -delete, it doesn't spawn any command, it just uses unlinkat().
Didn't know that either, thanks! Will change it too!
%find_lang %{name} --all-name
I think --all-name is not needed, but not sure.
I replaced it with %{name}-%{apiver}
%{_datadir}/gtk-doc/html/%{name}-0.4
%doc %{_datadir}/gtk-doc/html/%{name}-0.4
OK
%dir %{_datadir}/gtk-doc %dir %{_datadir}/gtk-doc/html
you don't need to do this.
I think I do - otherwise the directory ownership guideline [2] might be violated I think. lasem puts files in %{_datadir}/gtk-doc but does not depend on it, so it needs to own it. %doc only owns the lasem-0.4 folder.
$RPM_BUILD_ROOT
%{buildroot}
I prefer the variable format and as per current guidelines [1] both are acceptable.
hopefully it will be deprecated at some point. it's better to use %{buildroot}. but as you noted, it's up to you.
Let's stay with variable version then.
also would be great to do: %global apiver 0.4 and use it in %files.
OK
New releases: Spec URL: https://belegdol.fedorapeople.org/lasem/lasem.spec SRPM URL: https://belegdol.fedorapeople.org/lasem/lasem-0.4.3-2.fc24.src.rpm
[1] https://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot. 7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS
Resolution: ALMOST GOOD
New releases: Spec URL: https://belegdol.fedorapeople.org/lasem/lasem.spec SRPM URL: https://belegdol.fedorapeople.org/lasem/lasem-0.4.3-3.fc24.src.rpm
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_b...
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #5 from Igor Gnatenko ignatenko@redhat.com ---
lasem.x86_64: W: shared-lib-calls-exit /usr/lib64/liblasem-0.4.so.4.0.3 exit@GLIBC_2.2.5
probably can be filled to upstream
lasem.x86_64: E: incorrect-fsf-address /usr/share/licenses/lasem/COPYING
MUST be filled bug to upstream. Every file in src/ has incorrect FSF address
I think I do - otherwise the directory ownership guideline [2] might be violated I think. lasem puts files in %{_datadir}/gtk-doc but does not depend on it, so it needs to own it. %doc only owns the lasem-0.4 folder.
I think with %doc it's not needed.
There is bundled itex2mml in sources, add following: Licese: LGPLv2+ and GPLv2+ Provides: bundled(itex2mml) = 1.4.5 %license itex2mml/COPYING.itex2mml
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #6 from Julian Sikorski belegdol@gmail.com --- (In reply to Igor Gnatenko from comment #5)
lasem.x86_64: W: shared-lib-calls-exit /usr/lib64/liblasem-0.4.so.4.0.3 exit@GLIBC_2.2.5
probably can be filled to upstream
https://bugzilla.gnome.org/show_bug.cgi?id=768591
lasem.x86_64: E: incorrect-fsf-address /usr/share/licenses/lasem/COPYING
MUST be filled bug to upstream. Every file in src/ has incorrect FSF address
https://bugzilla.gnome.org/show_bug.cgi?id=768590
I think I do - otherwise the directory ownership guideline [2] might be violated I think. lasem puts files in %{_datadir}/gtk-doc but does not depend on it, so it needs to own it. %doc only owns the lasem-0.4 folder.
I think with %doc it's not needed.
Why? %doc or no %doc, files are getting placed in a folder which could be left dangling if package is uninstalled.
There is bundled itex2mml in sources, add following: Licese: LGPLv2+ and GPLv2+
Not sure about this one... As per COPYING.itex2mml, itex2mml is tri-licensed: GPL, MPL and LGPL, with no version specified. On the other hand, licensecheck says it's GPLv3+
Provides: bundled(itex2mml) = 1.4.5 %license itex2mml/COPYING.itex2mml
Agreed
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #7 from Julian Sikorski belegdol@gmail.com --- New releases: Spec URL: https://belegdol.fedorapeople.org/lasem/lasem.spec SRPM URL: https://belegdol.fedorapeople.org/lasem/lasem-0.4.3-4.fc24.src.rpm
I added itex2mml elements. Is there anything else?
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Igor Gnatenko ignatenko@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #8 from Igor Gnatenko ignatenko@redhat.com --- * $RPM_BUILD_ROOT -> %{buildroot}
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #9 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/lasem
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #10 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-0580653585
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #11 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-406d38c5ef
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #12 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-4060940fc7
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #13 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.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-0580653585
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #14 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.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-406d38c5ef
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #15 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc23 has been pushed to the Fedora 23 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-4060940fc7
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #16 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #17 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2016-09-09 12:53:59
https://bugzilla.redhat.com/show_bug.cgi?id=1352408
--- Comment #18 from Fedora Update System updates@fedoraproject.org --- lasem-0.4.3-4.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.
package-review@lists.fedoraproject.org