[Bug 1858531] New: Review Request: partio- Library for reading/writing/manipulating common animation particle

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

Bug ID: 1858531
           Summary: Review Request: partio- Library for
                    reading/writing/manipulating common animation particle
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

Spec URL:
https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/...
SRPM URL:
https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/...
Description: C++ (with python bindings) library for easily
reading/writing/manipulating common animation particle formats such as PDB,
BGEO, PTC.
Fedora Account System Username: luya

[Bug 1858531] Review Request: partio- Library for reading/writing/manipulating common animation particle

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

Luya Tshimbalanga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1856589
           Doc Type|---                         |If docs needed, set a value

Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=1856589
[Bug 1856589] Review Request: openshadinglanguage - Advanced shading language
for production GI renderers

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #2 from Robert-André Mauchin 🐧  ---
Sorry I shouldn't have checked "do not send mail". Please see the comment
above.

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #3 from Robert-André Mauchin 🐧  ---
Also the tests contain arch-dependent binaries in /usr/share, which is not
good. /usr/share is for arch independent code only:

partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/makecircle
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/makeline
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testcache
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testclonecopy
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testcluster
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testio
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testiterator
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testkdtree
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/testmerge
partio-test.x86_64: E: arch-dependent-file-in-usr-share
/usr/share/partio/test/teststr

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #4 from Luya Tshimbalanga  ---
(In reply to Robert-André Mauchin 🐧 from comment #1)
> Please fix the Source0:
>
> Source0:       
> https://github.com/wdas/%%7Bname%7D/archive/v%%7Bversion%7D/%%7Bname%7D-%%7B...
>
> (archives → archive)

Fixed.
> That should be in the Python subpackage:
>
> %{python3_sitearch}/_%{name}.so

Fixed,
> (Probably something like that:
>
> set_target_properties(partio PROPERTIES
>     OUTPUT_NAME partio POSITION_INDEPENDENT_CODE ON
>     VERSION ${VERSION}
>     SOVERSION 1 
> )
>
> in src/lib/CMakeLists.txt)

After applying a patch using the above code, the build managed to build as seen
on the scratch build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=47414693

I filed a ticket to upstream about the versioning libraries:
https://github.com/wdas/partio/issues/82
> You should not provide %license LICENSE for all packages, but for all
>
> packages combination. For ex, -devel depends on the main package which
> already provide the license, so it shouldn't be included another time in the
> -devel subpackage. Same with -doc subpackage.

Fixed.
> Is it useful to package the tests? Are they used by the end-user?

Those tests aren't need so they are removed.
> Add Version-Release to your changelog entry.

Done.

Here is the updated files:
SPEC:
https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/...
SRPM:
https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/...

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #5 from Robert-André Mauchin 🐧  ---
Yeah I have not tested that patch at all. I suspect it is because the $VERSION
variable is not available at this point on the build. I'm not an expert of
Cmake, so I don't really know how to handle this.

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #6 from Robert-André Mauchin 🐧  ---
Fixed it.

diff -ru partio-1.10.1.orig/src/lib/CMakeLists.txt
partio-1.10.1/src/lib/CMakeLists.txt
--- partio-1.10.1.orig/src/lib/CMakeLists.txt   2019-07-10 19:11:02.000000000
-0700
+++ partio-1.10.1/src/lib/CMakeLists.txt        2020-07-18 20:50:13.713384480
-0700
@@ -35,7 +35,11 @@
 file(GLOB core_cpp "core/*.cpp")

add_library(partio ${io_cpp} ${core_cpp})
-set_target_properties(partio PROPERTIES OUTPUT_NAME partio
POSITION_INDEPENDENT_CODE ON)
+set_target_properties(partio PROPERTIES 
+    OUTPUT_NAME partio POSITION_INDEPENDENT_CODE ON
+    VERSION ${CMAKE_PROJECT_VERSION}
+    SOVERSION 1 
+)

target_include_directories(partio
     PUBLIC

================================================================

No the other issue I haven't mentioned yet is where to put the library we
created. I think it should ship separately into a libpartio subpackage and then
you change your devel package into libpartio-devel and make libpartio-devel
requires libpartio instead of the main package. You might need to adjust the
Summary/Description to reflect what's actually packaged (i.e. main package
containing the tools and libpartio containing the lib)

%files -n libpartio
%license LICENSE
%{_libdir}/*.so.1*

- Also remove the tabs introduced in line 12, line 55, line 58

- Do not use ExcludeArch but ExclusiveArch with the arches you support.

-

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #7 from Luya Tshimbalanga  ---
Thank you for the fix. The build is successful this time. 
I figured out some build requirements lack pkgconfig files on Fedora 31 when
testing the scratch build so I adjusted the condition.

(In reply to Robert-André Mauchin 🐧 from comment #6)
> libpartio-devel requires libpartio instead of the main package. You might
> need to adjust the Summary/Description to reflect what's actually packaged
> (i.e. main package containing the tools and libpartio containing the lib)
>
> %files -n libpartio
> %license LICENSE
> %{_libdir}/*.so.1*

Is "%files lib" an equivalent partio-devel requires partio-lib? Some packages
used that method.
> Also remove the tabs introduced in line 12, line 55, line 58

Fixed
> Do not use ExcludeArch but ExclusiveArch with the arches you support.

I find out it is unneeded.

Here is the updated files:

SPEC:
https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/...
SRPM:
https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/...

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

Robert-André Mauchin 🐧  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |POST
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review+

--- Comment #8 from Robert-André Mauchin 🐧  ---
> Is "%files lib" an equivalent partio-devel requires partio-lib? Some packages used that method.

Yes that is a solution, I'm not sure what is the standard in Fedora.

Package approved.

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #9 from Luya Tshimbalanga  ---
Thank you Robert-André!!

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

--- Comment #10 from Gwyn Ciesla  ---
(fedscm-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/partio

[Bug 1858531] Review Request: partio - Library for reading/writing/manipulating common animation particle

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

Luya Tshimbalanga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |CLOSED
         Resolution|---                         |CURRENTRELEASE
        Last Closed|                            |2020-09-06 05:43:14

--- Comment #11 from Luya Tshimbalanga  ---
Closing as the package is available in the repository.