Bug 1858531 - Review Request: partio - Library for reading/writing/manipulating common animation particle
Summary: Review Request: partio - Library for reading/writing/manipulating common anim...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1856589
TreeView+ depends on / blocked
 
Reported: 2020-07-18 20:40 UTC by Luya Tshimbalanga
Modified: 2020-09-06 05:43 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-09-06 05:43:14 UTC
Type: ---
Embargoed:
eclipseo: fedora-review+


Attachments (Terms of Use)

Description Luya Tshimbalanga 2020-07-18 20:40:54 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/fedora-32-x86_64/01559518-partio/partio.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/fedora-32-x86_64/01559518-partio/partio-1.10.1-1.fc32.src.rpm
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

Comment 1 Robert-André Mauchin 🐧 2020-07-18 22:54:23 UTC
 - Please fix the Source0:

Source0:        https://github.com/wdas/%{name}/archive/v%{version}/%{name}-%{version}.tar.gz

(archives → archive)

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

 - There is no versioned library, only the unversioned. You must provide a versioned library, by first asking upstream to version their library, or, in case of refusal, do the versioning dowstream. See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_downstream_so_name_versioning

(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)


 - 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.

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

 - Add Version-Release to your changelog entry.

Comment 2 Robert-André Mauchin 🐧 2020-07-18 22:55:19 UTC
Sorry I shouldn't have checked "do not send mail". Please see the comment above.

Comment 3 Robert-André Mauchin 🐧 2020-07-18 23:00:11 UTC
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

Comment 4 Luya Tshimbalanga 2020-07-19 05:46:09 UTC
(In reply to Robert-André Mauchin 🐧 from comment #1)
>  - Please fix the Source0:
> 
> Source0:       
> https://github.com/wdas/%{name}/archive/v%{version}/%{name}-%{version}.tar.gz
> 
> (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/fedora-rawhide-x86_64/01559730-partio/partio.spec
SRPM: https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/fedora-rawhide-x86_64/01559730-partio/partio-1.10.1-2.fc33.src.rpm

Comment 5 Robert-André Mauchin 🐧 2020-07-19 09:32:29 UTC
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.

Comment 6 Robert-André Mauchin 🐧 2020-07-19 09:55:07 UTC
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.

 -

Comment 7 Luya Tshimbalanga 2020-07-19 18:14:44 UTC
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/fedora-32-x86_64/01563228-partio/partio.spec
SRPM: https://download.copr.fedorainfracloud.org/results/luya/openshadinglanguage/fedora-32-x86_64/01563228-partio/partio-1.10.1-3.fc32.src.rpm

Comment 8 Robert-André Mauchin 🐧 2020-07-19 20:46:53 UTC
> 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.

Comment 9 Luya Tshimbalanga 2020-07-20 05:19:34 UTC
Thank you Robert-André!!

Comment 10 Gwyn Ciesla 2020-07-20 13:53:05 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/partio

Comment 11 Luya Tshimbalanga 2020-09-06 05:43:14 UTC
Closing as the package is available in the repository.


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