> Requires:       pytside2-tools

typo



> %package -n     python3-%{pypi_name}-devel

For consistency, this should provide camel_name + do %python_provide as well.
Other subpackages don't %python_provide either.



> # Fix all Python shebangs recursively in .

This is copy-pasted, but the . part is no longer true.

> pathfix.py -pni "%{__python3} %{py3_shbang_opts}" %{buildroot}%{_bindir}/*.py
> pathfix.py -pni "%{__python3} %{py3_shbang_opts}" %{buildroot}%{_bindir}/pyside2-uic

Just do: pathfix.py -pni "%{__python3} %{py3_shbang_opts}" %{buildroot}%{_bindir}/*
It will be future proof.


> # Lots of tests fail currently...
> #{__python3} testrunner.py test

If I decide to check this, how long does the package build? Will it eat all my RAM and soul?

(In reply to Miro Hrončok from comment #1)
> > Requires:       pytside2-tools
> 
> typo

Fixed.


> > %package -n     python3-%{pypi_name}-devel
> 
> For consistency, this should provide camel_name + do %python_provide as well.
> Other subpackages don't %python_provide either.

Wasn't sure exactly what you meant so I added both python_provides just like the main module.


> > # Fix all Python shebangs recursively in .
> 
> This is copy-pasted, but the . part is no longer true.
> 
> > pathfix.py -pni "%{__python3} %{py3_shbang_opts}" %{buildroot}%{_bindir}/*.py
> > pathfix.py -pni "%{__python3} %{py3_shbang_opts}" %{buildroot}%{_bindir}/pyside2-uic
> 
> Just do: pathfix.py -pni "%{__python3} %{py3_shbang_opts}"
> %{buildroot}%{_bindir}/*
> It will be future proof.

Fixed.


> > # Lots of tests fail currently...
> > #{__python3} testrunner.py test
> 
> If I decide to check this, how long does the package build? Will it eat all
> my RAM and soul?

It does increase the build time significantly but I don't think it's too resource hungry. My desktop (AMD Ryzen 5 2600 w/ 16GB RAM) handles it fine.

Ok, correction, testing doesn't appear to work with the direct CMake build method.

Richard, I looked through the history of this in bug 1634658 to get up to speed.  Thanks for your hard work on this so far.  You said you've fixed those things Miro pointed out, but your original spec file link doesn't have those fixes.  Is it at a new URL now?


Can you adjust your globs in %files to be more specific?  Perhaps something like:

-%{_libdir}/libpyside2*.so.*
+%{_libdir}/libpyside2*.so.5.12*

-%{_libdir}/libshiboken2*.so.*
+%{_libdir}/libshiboken2*.so.5.12*

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files


I also agree with Miro's previous point about filtering the autoprovides (bug 1634658, comment 90).  I checked a few other arched python modules, and none of them provide their sonames from within %{python3_sitearch}.

https://docs.fedoraproject.org/en-US/packaging-guidelines/AutoProvidesAndRequiresFiltering/#_summary

> Packages must not provide RPM dependency information when that information is not global in nature

Since libpyside2.cpython-37m-x86_64-linux-gnu.so.5.12 and libshiboken2.cpython-37m-x86_64-linux-gnu.so.5.12 are not global in nature I think they should be filtered out.  As further evidence of this, rpmlint expects python3-shiboken2-devel to explicitly require python3-shiboken2-libs.

> python3-shiboken2-devel.x86_64: W: no-dependency-on python3-shiboken2/python3-shiboken2-libs/libpython3-shiboken2

I would filter out the automatic libshiboken2 provides/requires and explicitly require the necessary packages.

I haven't posted a new spec/srpm since it wasn't a formal review but I can do that. 

I can make the library globs more specific but PySide2 is tied to the Qt 5 version so if Qt 5 gets updated to some > 5.12, PySide2 MUST be updated as well, so I'd be keeping a close eye on it anyway.

I'll add the filtering and do a test build. If it works as expected I'll post new spec and srpm links.

(In reply to Carl George from comment #4)
> 
> Can you adjust your globs in %files to be more specific?  Perhaps something
> like:
> 
> -%{_libdir}/libpyside2*.so.*
> +%{_libdir}/libpyside2*.so.5.12*
> 
> -%{_libdir}/libshiboken2*.so.*
> +%{_libdir}/libshiboken2*.so.5.12*
> 
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> #_listing_shared_library_files

Done.


> I also agree with Miro's previous point about filtering the autoprovides
> (bug 1634658, comment 90).  I checked a few other arched python modules, and
> none of them provide their sonames from within %{python3_sitearch}.
> 
> https://docs.fedoraproject.org/en-US/packaging-guidelines/
> AutoProvidesAndRequiresFiltering/#_summary
> 
> > Packages must not provide RPM dependency information when that information is not global in nature
> 
> Since libpyside2.cpython-37m-x86_64-linux-gnu.so.5.12 and
> libshiboken2.cpython-37m-x86_64-linux-gnu.so.5.12 are not global in nature I
> think they should be filtered out.  As further evidence of this, rpmlint
> expects python3-shiboken2-devel to explicitly require python3-shiboken2-libs.

The files are actually installed to %{_libdir} not %{python3_sitearch} so per the conversation in the devel mailing list I think I'm going to leave them be.


> > python3-shiboken2-devel.x86_64: W: no-dependency-on python3-shiboken2/python3-shiboken2-libs/libpython3-shiboken2
> 
> I would filter out the automatic libshiboken2 provides/requires and
> explicitly require the necessary packages.

I went ahead and added explicit requires on both shiboken2 and shiboken2-libs.

I'm going to do a test build first and then I'll post new links.

SPEC: https://hobbes1069.fedorapeople.org/python-pyside2.spec
SRPM: https://hobbes1069.fedorapeople.org/python-pyside2-5.12.1-3.fc29.src.rpm

* Tue Apr 23 2019 Richard Shaw <hobbes1069> - 5.12.1-3
- Update per review comments.
- Make library globs dependent on soname.
- Add explicit requires for skiboken2-libs and shibokwn2 on shiboken2-devel.
- Try to workaround qt5-qtwebengine not being available on ppc64le and s390x.

https://koji.fedoraproject.org/koji/taskinfo?taskID=34364683

Looks like ppc64le and s390x build fine without qt5-qtwebengine.

> so per the conversation in the devel mailing list I think I'm going to leave them be.

For posterity: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/UPAU75JYH7MNPEQ2TQD7YX6NIUPZSCT3/


> I went ahead and added explicit requires on both shiboken2 and shiboken2-libs.

> Requires:       python3-shiboken2
> Requires:       python3-shiboken2-libs

What provides python3-shiboken2?  I don't see that in the spec file, either as a subpackage or as a virtual provides.  Speaking of, why are python3-pyside2 and python3-shiboken2-libs not named similarly?  Both have similar contents, so at first glance it seem that python3-shiboken2-libs should just be named python3-shiboken2.  Let me know if I'm missing something here.

> [ ]: License file installed when any subpackage combination is installed.

All the licenses are in the python3-pyside2 package, but installing other packages doesn't always pull it in as a dependency.  If the dependencies are correct, then the different subpackages should all own the license files.

(In reply to Carl George from comment #9)
> 
> > I went ahead and added explicit requires on both shiboken2 and shiboken2-libs.
> 
> > Requires:       python3-shiboken2
> > Requires:       python3-shiboken2-libs
> 
> What provides python3-shiboken2?  I don't see that in the spec file, either
> as a subpackage or as a virtual provides.  Speaking of, why are
> python3-pyside2 and python3-shiboken2-libs not named similarly?  Both have
> similar contents, so at first glance it seem that python3-shiboken2-libs
> should just be named python3-shiboken2.  Let me know if I'm missing
> something here.

Whoops, fixed the first one to just shiboken2.

I was trying to mimic the original pyside packaging as much as possible, but I could be convinced otherwise.


(In reply to Carl George from comment #10)
> > [ ]: License file installed when any subpackage combination is installed.
> 
> All the licenses are in the python3-pyside2 package, but installing other
> packages doesn't always pull it in as a dependency.  If the dependencies are
> correct, then the different subpackages should all own the license files.

Yeah, I dumped them in there initially. I'm going through the source files now to see which is which.

Bah, something has changed in the build environment and now builds are failing...

$ grep "error:" build.log 
BUILDSTDERR: /builddir/build/BUILD/pyside-setup-everywhere-src-5.12.1/sources/pyside2/libpyside/dynamicqmetaobject.cpp:55:10: fatal error: 'private/qmetaobjectbuilder_p.h' file not found
BUILDSTDERR: /usr/include/qt5/QtQml/5.12.1/QtQml/private/qtqmlglobal_p.h:54:10: fatal error: 'QtCore/private/qglobal_p.h' file not found
BUILDSTDERR: error: Bad exit status from /var/tmp/rpm-tmp.TxpUyZ (%build)

The build shows "-DPYSIDE_QML_PRIVATE_API_SUPPORT=1" on the build command...

Fixed it, don't know what changed but I had to add the package for the private headers which I haven't had to do before...

SPEC: https://hobbes1069.fedorapeople.org/python-pyside2.spec
SRPM: https://hobbes1069.fedorapeople.org/python-pyside2-5.12.1-3.fc29.src.rpm

* Tue Apr 23 2019 Richard Shaw <hobbes1069> - 5.12.1-3
- Update per review comments.
- Make library globs dependent  on soname.
- Add explicit requires for skiboken2-libs and shibokwn2 on shiboken2-devel.
- Try to workaround qt5-qtwebengine not being available on ppc64le and s390x.

Thanks Richard for fixing the requirement and license concerns I had.  I'm ready to approve this package, I just wanted to get your feedback on the python3-shiboken2-libs naming question first.

> Speaking of, why are python3-pyside2 and python3-shiboken2-libs not named similarly?  Both have similar contents, so at first glance it seem that python3-shiboken2-libs should just be named python3-shiboken2.  Let me know if I'm missing something here.

(In reply to Carl George from comment #14)
> Thanks Richard for fixing the requirement and license concerns I had.  I'm
> ready to approve this package, I just wanted to get your feedback on the
> python3-shiboken2-libs naming question first.
> 
> > Speaking of, why are python3-pyside2 and python3-shiboken2-libs not named similarly?  Both have similar contents, so at first glance it seem that python3-shiboken2-libs should just be named python3-shiboken2.  Let me know if I'm missing something here.

Yeah, I was kinda following the Pyside / shiboken 1 naming scheme there but I don't think it's necessary. I'll fix that. I can post new links if you like.

Yes please, go ahead and make that change, post the links, and I'll run fedora-review on it one more time to get the proper review template pasted in here.

I tried doing a scratch build myself and it failed as someone is upgrading all of qt5 to 5.12.3 in Rawhide. Hopefully a local mock build will work.

A local mock build succeeds. In either case, I will have to update to 5.12.3 when Jan finishes his work[1] since it has to stay in sync.

SPEC: https://hobbes1069.fedorapeople.org/python-pyside2.spec
SRPM: https://hobbes1069.fedorapeople.org/python-pyside2-5.12.1-4.fc29.src.rpm

* Tue Apr 23 2019 Richard Shaw <hobbes1069> - 5.12.1-3
- Update per review comments.
- Make library globs dependent  on soname.
- Add explicit requires for skiboken2-libs and shibokwn2 on shiboken2-devel.
- Try to workaround qt5-qtwebengine not being available on ppc64le and s390x.

[1] https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/Q7ZAG6LTGPQNXYNIS3L2CPZ6VR3HNZAB/

Posted the wrong changelog:

* Tue Jun 04 2019 Richard Shaw <hobbes1069> - 5.12.1-4
- Change python3-shiboken-libs to python3-shiboken.

Package approved.

There are a few other "should" items that can be cleaned up, but I don't want to hold up the review any longer for them.  Fix them at your leisure.

* Enable the test suite in the future if you are able.
* I don't see what your jobs macro is used for, and it's throwing a warning for using define instead of global.  Remove it if you can, or switch it to global.
* /usr/lib64/python3.7/site-packages/pyside2uic/icon_cache.py is throwing some warnings due to it's shebang line.  Strip that out if you can.




Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 61440 bytes in 6 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Packages MUST NOT have dependencies (either build-time or runtime) on
     packages named with the unversioned python- prefix unless no properly
     versioned package exists. Dependencies on Python packages instead MUST
     use names beginning with python2- or python3- as appropriate.
[x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
[x]: Package functions as described.
[-]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[!]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %{?_smp_mflags: %define jobs
     %(echo %{_smp_mflags} | sed 's/-j//')}
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 1126400 bytes in /usr/share
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: python3-pyside2-5.12.1-4.fc31.x86_64.rpm
          python3-pyside2-devel-5.12.1-4.fc31.x86_64.rpm
          pyside2-tools-5.12.1-4.fc31.x86_64.rpm
          shiboken2-5.12.1-4.fc31.x86_64.rpm
          python3-shiboken2-5.12.1-4.fc31.x86_64.rpm
          python3-shiboken2-devel-5.12.1-4.fc31.x86_64.rpm
          python-pyside2-debuginfo-5.12.1-4.fc31.x86_64.rpm
          python-pyside2-debugsource-5.12.1-4.fc31.x86_64.rpm
          python-pyside2-5.12.1-4.fc31.src.rpm
python3-pyside2-devel.x86_64: W: no-dependency-on python3-pyside2/python3-pyside2-libs/libpython3-pyside2
python3-pyside2-devel.x86_64: W: no-documentation
pyside2-tools.x86_64: W: spelling-error %description -l en_US rcc -> cc, rec, r cc
pyside2-tools.x86_64: W: spelling-error %description -l en_US uic -> uric, sic, tic
pyside2-tools.x86_64: W: spelling-error %description -l en_US lupdate -> update, l update, elucidate
pyside2-tools.x86_64: E: wrong-script-interpreter /usr/lib64/python3.7/site-packages/pyside2uic/icon_cache.py /usr/bin/env python
pyside2-tools.x86_64: E: non-executable-script /usr/lib64/python3.7/site-packages/pyside2uic/icon_cache.py 644 /usr/bin/env python
pyside2-tools.x86_64: W: no-manual-page-for-binary pyside_tool.py
shiboken2.x86_64: W: spelling-error %description -l en_US Shiboken -> Shinbone
shiboken2.x86_64: W: no-manual-page-for-binary shiboken_tool.py
python3-shiboken2.x86_64: W: spelling-error %description -l en_US Shiboken -> Shinbone
python3-shiboken2.x86_64: W: shared-lib-calls-exit /usr/lib64/libshiboken2.cpython-37m-x86_64-linux-gnu.so.5.12.1 exit.5
python3-shiboken2-devel.x86_64: W: no-version-dependency-on python3-shiboken2/python3-shiboken2-libs/libpython3-shiboken2 5.12.1
python3-shiboken2-devel.x86_64: W: spelling-error %description -l en_US Shiboken -> Shinbone
9 packages and 0 specfiles checked; 2 errors, 12 warnings.




Rpmlint (debuginfo)
-------------------
Checking: python-pyside2-debuginfo-5.12.1-4.fc31.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

(In reply to Carl George from comment #20)
> Package approved.
> 
> There are a few other "should" items that can be cleaned up, but I don't
> want to hold up the review any longer for them.  Fix them at your leisure.
> 
> * Enable the test suite in the future if you are able.

I'll continue to check if it works on new releases but it doesn't appear to be compatible with the direct cmake build method.


> * I don't see what your jobs macro is used for, and it's throwing a warning
> for using define instead of global.  Remove it if you can, or switch it to
> global.

That's left over from when I was using the setuptools method which didn't support -j. I can remove it.


> * /usr/lib64/python3.7/site-packages/pyside2uic/icon_cache.py is throwing
> some warnings due to it's shebang line.  Strip that out if you can.

If I new whether it was expected to be run directly I might just remove it, for now I can add it to the path fixes script.

Thanks for the review!

I recommend also test building this beast against https://copr.fedorainfracloud.org/coprs/g/python/python3.8/ to see if it still works there.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/python-pyside2

Package built for Rawhide. I can't test py38 until a new rawhide compose completes since a dependent qt 5.12.3 package was missing making the py38 COPR repo unusable.

I'll also build for F30 once 5.12.3 makes it there.

Thanks everyone!

The copr repo only works with  --enablerepo=local in mock.

I'm on traveling on vacation through next week but I may have time to try that tomorrow.