Bug 2072752 - Review Request: tinyexr - small library to load and save OpenEXR images
Summary: Review Request: tinyexr - small library to load and save OpenEXR images
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-04-06 21:54 UTC by Diego Herrera
Modified: 2022-04-20 21:09 UTC (History)
2 users (show)

Fixed In Version: tinyexr-1.0.1-1.fc37
Clone Of:
Environment:
Last Closed: 2022-04-20 21:08:33 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Diego Herrera 2022-04-06 21:54:17 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/dherrera/tinyexr/fedora-rawhide-x86_64/04083963-tinyexr/tinyexr.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/dherrera/tinyexr/fedora-rawhide-x86_64/04083963-tinyexr/tinyexr-1.0.1-1.fc37.src.rpm

Description: TinyEXR is a small library to load and save OpenEXR images. It supports the version 1 format and version 2 multi-part images, and it has partial support for version 2 deep images.

Fedora Account System Username: dherrera

Comment 1 Zbigniew Jędrzejewski-Szmek 2022-04-10 10:18:21 UTC
> Release:        1%{?dist}
> %changelog
> ...

Suggestion: https://docs.pagure.org/fedora-infra.rpmautospec/index.html

> mkdir -p ${RPM_BUILD_ROOT}%{_datadir}/licenses/%{name}/
> install -p -m 644 %{SOURCE1} \
>    ${RPM_BUILD_ROOT}%{_datadir}/licenses/%{name}/LICENSE

I don't think you need to do this. Just '%license %{SOURCE1}' should be enough.

fedora-review says:
- Development (unversioned) .so files in -devel subpackage, if present.
  Note: Unversioned so-files directly in %_libdir.
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_devel_packages

The %files pattern seems wrong: %{_libdir}/libtinyexr.so should be in -devel.

Hmm, if I go to https://www.openexr.com/, they link to https://github.com/AcademySoftwareFoundation/openexr,
which has version 3.1.4 as the latest. https://github.com/syoyo/tinyexr seems to be a dead project.

Comment 2 Diego Herrera 2022-04-12 00:25:40 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/dherrera/tinyexr/fedora-36-x86_64/04181151-tinyexr/tinyexr-1.0.1-5.fc36.src.rpm
SRPM URL: https://download.copr.fedorainfracloud.org/results/dherrera/tinyexr/fedora-36-x86_64/04181151-tinyexr/tinyexr.spec

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> > Release:        1%{?dist}
> > %changelog
> > ...
> 
> Suggestion: https://docs.pagure.org/fedora-infra.rpmautospec/index.html

Took your sugestion, I was planning to learn about it, found out this was the perfect ocasion, thx for the push.

> 
> > mkdir -p ${RPM_BUILD_ROOT}%{_datadir}/licenses/%{name}/
> > install -p -m 644 %{SOURCE1} \
> >    ${RPM_BUILD_ROOT}%{_datadir}/licenses/%{name}/LICENSE
> 
> I don't think you need to do this. Just '%license %{SOURCE1}' should be
> enough.
> 

Cleaned up this part.

> fedora-review says:
> - Development (unversioned) .so files in -devel subpackage, if present.
>   Note: Unversioned so-files directly in %_libdir.
>   See: https://docs.fedoraproject.org/en-US/packaging-
>   guidelines/#_devel_packages
> 
> The %files pattern seems wrong: %{_libdir}/libtinyexr.so should be in -devel.
> 

Fixed!

> Hmm, if I go to https://www.openexr.com/, they link to
> https://github.com/AcademySoftwareFoundation/openexr,
> which has version 3.1.4 as the latest. https://github.com/syoyo/tinyexr
> seems to be a dead project.

I mailed the maintainer and he is still active, also I think it's a good idea to package it since the Godot project already uses it (this library is actually packaged there as an internal library).

My main interest though is to get it so I can update the Opentoonz package, since this is a dependency for the current stable version.

Comment 3 Zbigniew Jędrzejewski-Szmek 2022-04-12 06:41:33 UTC
OK. The two repositories look quite different indeed. But I don't think openexr.com
should be used as URL: maybe use it in %description instead. The URL: field is for
the *project*.

> ## START: Set by rpmautospec
> ...

Whoah, where did that come from? I haven't seen something like that before…

> Took your sugestion, I was planning to learn about it, found out this was the perfect ocasion, thx for the push.

Hmm, and what about %autochangelog?
One without the other is possible, but the whole point is make this as simple as possible.
There is no reason to have a 'changelog' file in a *new* pacakge. The whole thing should be:

Name:           tinyexr
Version:        1.0.1
Release:        %autorelease
...
%changelog
%autochangelog

(No other lines related to version, release or changelog.)

Comment 4 Diego Herrera 2022-04-12 13:55:07 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/dherrera/tinyexr/fedora-rawhide-x86_64/04194342-tinyexr/tinyexr.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/dherrera/tinyexr/fedora-rawhide-x86_64/04194342-tinyexr/tinyexr-1.0.1-6.fc37.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #3)
> OK. The two repositories look quite different indeed. But I don't think
> openexr.com
> should be used as URL: maybe use it in %description instead. The URL: field
> is for
> the *project*.

Fixed

> 
> > ## START: Set by rpmautospec
> > ...
> 
> Whoah, where did that come from? I haven't seen something like that before…
> 
> > Took your sugestion, I was planning to learn about it, found out this was the perfect ocasion, thx for the push.
> 
> Hmm, and what about %autochangelog?
> One without the other is possible, but the whole point is make this as
> simple as possible.
> There is no reason to have a 'changelog' file in a *new* pacakge. The whole
> thing should be:
> 
> Name:           tinyexr
> Version:        1.0.1
> Release:        %autorelease
> ...
> %changelog
> %autochangelog
> 
> (No other lines related to version, release or changelog.)

I actually did that, but I messed up by uploading to COPR the srpm that I generated on my PC using mock.

Had to change my workflow a bit to get it right this time, but it's for the better :).

Thank you for the help and patience.

Comment 5 Zbigniew Jędrzejewski-Szmek 2022-04-12 14:30:33 UTC
+ package name is correct
+ latest version
+ license is acceptable for Fedora (BSD)
+ license is specified correctly
+ builds and installs OK

rpmlint says:
tinyexr-debuginfo.x86_64: W: unstripped-binary-or-object /usr/lib/debug/usr/lib64/libtinyexr.so.1.0.1-1.0.1-6.fc37.x86_64.debug
tinyexr-debuginfo.x86_64: E: shared-library-without-dependency-information /usr/lib/debug/usr/lib64/libtinyexr.so.1.0.1-1.0.1-6.fc37.x86_64.debug
tinyexr-debuginfo.x86_64: W: no-documentation
tinyexr-debugsource.x86_64: W: no-documentation
tinyexr-devel.x86_64: W: no-documentation
tinyexr-debuginfo.x86_64: W: dangling-relative-symlink /usr/lib/debug/.build-id/11/c10a27235719c56f8ef037e28e532bc59b891e ../../../.build-id/11/c10a27235719c56f8ef037e28e532bc59b891e
^ all bogus

tinyexr.src: W: name-repeated-in-summary TinyEXR
tinyexr.x86_64: W: name-repeated-in-summary TinyEXR
That's a valid point: s/TinyEXR is a s/S/

Package is APPROVED.

Maybe drop the 'changelog' file and '-b 6'? Those changelog entries before
the package is approved are not important.

Comment 6 Gwyn Ciesla 2022-04-13 13:34:02 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/tinyexr


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