https://bugzilla.redhat.com/show_bug.cgi?id=1895567 --- Comment #15 from Luya Tshimbalanga <luya_tfz@xxxxxxxxxxxxxxxx> --- (In reply to Ben Beasley from comment #14) > I’m not quite ready to commit to being the reviewer for this package. > Please consider this a “preliminary review” in advance of review by myself > or by someone else. Thank for the detaied preliminary review > > There’s a lot going on in this package, and there is room for me to have > missed, or to be wrong about, something below, or for reasonable disagreement > about any number of details. > > - Reported by fedora-review: > Header files in -devel subpackage, if present. > Note: usd : /usr/share/usd/examples/include/pxr/imaging/hdTiny/mesh.h usd > : /usr/share/usd/examples/include/pxr/imaging/hdTiny/renderDelegate.h usd > : /usr/share/usd/examples/include/pxr/imaging/hdTiny/renderPass.h usd : > /usr/share/usd/examples/include/pxr/imaging/hdTiny/rendererPlugin.h usd- > libs : /usr/lib64/usd/usd/resources/codegenTemplates/api.h usd-libs : > /usr/lib64/usd/usd/resources/codegenTemplates/schemaClass.h usd-libs : > /usr/lib64/usd/usd/resources/codegenTemplates/tokens.h > See: https://docs.fedoraproject.org/en-US/packaging- > guidelines/#_devel_packages > > The examples are certainly okay, and the others are used as templates for > code generation, so I think this is fine. > > - The shared libraries do not have SONAMEs set; the unversioned .so files > packaged in -devel are the actual libraries that should be in -libs, and > there are no libraries in -libs at all! > > There was some previous discussion of this, but I am still not convinced > this is right. These libraries correspond to the headers in -devel; if > those are public headers then these are public libraries and should be > installed in -libs. And in that case, they need so-versions and symlinks. > At least, that’s how it seems. > > Make sure to update the versions on any virtual Provides when you update > the > overall package. Done. Ticket filed upstream on https://github.com/PixarAnimationStudios/USD/issues/1490 > > - There are parts of the package with license other than the overall license > of > ASL 2.0; you must follow > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#_multiple_licensing_scenarios, > updating the License field and providing a breakdown of licensing. See the > linked guidelines section for three ways to do this. Note that even the > main > LICENSE.txt describes components under multiple licenses. > > The overall license appears to be > > ASL 2.0 and BSD and MIT and (MIT or Unlicense) > > as broken down in the following. > Fixed. > > - Are you intending to build python3-usd? Why is the build conditional > currently off? Note that this provides a lot more than usdview; it is also > required for quite a few command-line tools. > > I tried it and the build bailed out with > > -- Found PySide2 but NOT pyside-uic binary > CMake Error at > /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:165 (message): > Could NOT find PySide (missing: PYSIDE_AVAILABLE) > Call Stack (most recent call first): > /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:458 > (_FPHSA_FAILURE_MESSAGE) > cmake/modules/FindPySide.cmake:75 (find_package_handle_standard_args) > cmake/defaults/Packages.cmake:273 (find_package) > CMakeLists.txt:23 (include) > > but this is fixable; see > https://bugzilla.redhat.com/show_bug.cgi?id=1882270. You > can just change > -DPYSIDEUICBINARY="%{_bindir}/pyside2-uic" \ > to > -DPYSIDEUICBINARY="%{_bindir}/uic-qt5" \ > and add > BuildRequires: qt5-qtbase-devel > to the Python subpackage. Fixed with BuildRequires: pkgconfig(Qt5) which pull qt5-qtbase-devel. > > Of course, once you have a GUI application (usdview is one, I think?) you > will > need a .desktop file > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files > and AppData > https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/. Noted. > > Furthermore, it seems like the installation of the Python packages is > really, really broken > when I try to enable it, so I guess there would be more work needed if the > Python parts were > to be packaged. The python part failed with a syntax error on post installation below: Bytecompiling .py files below /builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9 using python3.9 *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/adjustClippingUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/adjustClippingUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/adjustDefaultMaterialUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/adjustDefaultMaterialUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/attributeValueEditorUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/attributeValueEditorUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/mainWindowUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/mainWindowUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/preferencesUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/preferencesUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/primLegendUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/primLegendUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax *** Error compiling '/builddir/build/BUILDROOT/usd-21.02-4.fc34.x86_64/usr/lib/python3.9/site-packages/pxr/Usdviewq/propertyLegendUI.py'... File "/usr/lib/python3.9/site-packages/pxr/Usdviewq/propertyLegendUI.py", line 1 /******************************************************************************** ^ SyntaxError: invalid syntax error: Bad exit status from /var/tmp/rpm-tmp.a2JBys (%install) Bad exit status from /var/tmp/rpm-tmp.a2JBys (%install) > > - Does the documentation build easily if you set > PXR_BUILD_DOCUMENTATION=TRUE and > add BR’s on doxygen and graphviz? A real -doc subpackage with full > documentation > would be a nice addition. Unfortunately the build failed with that setup so PXR_BUILD_DOCUMENTATION=FALSE for the time being. > - Above the ExclusiveArch, please try to explain in a little more detail why > the package must be x86_64-only. I agree that this looks like it is the > case. > Once the package is approved, you must create Bugzilla bugs blocking the > various ExcludeArch tracker bugs, even if you close them immediately as > WONTFIX. See > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_architecture_build_failures. Will do. > - Please change > > Requires: %{name}%{?_isa} = %{version} > > to > > Requires: %{name}%{?_isa} = %{version}-%{release} > > See > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_requiring_base_package. > > Note also that the -devel package should really require > > Requires: %{name}-libs%{?_isa} = %{version}-%{release} > > instead, and perhaps the base package should as well. Done > > - There is no %check section; did you try building the tests? Yes. Unfortunately, the provided upstream test failed to build. > - Please remove > > # Explicity define python macro to avoid unversioned python > %define __python /usr/bin/python3 > > because nothing should be using an unversioned %__python. (Also, prefer > %global to %define.) > > - Is Python really found using pkg-config? Maybe just drop this? > > BuildRequires: pkgconfig(python3) Good catch. Fixed. I standardize the use pkgconfig in the spec when possible so BuildRequires: pkgconfig(python3) fixed the issue. > - Note that you can use (but are not required to use) > https://src.fedoraproject.org/rpms/pyproject-rpm-macros to get > automatically > generated Python BR’s. Thanks for the info. It will help for future spec file update. > > - Please convert NOTICE.txt from CRNL line encoding. Add > > BuildRequires: dos2unix > > and in %prep, > > dos2unix NOTICE.txt Done > > - Please add > > find %{buildroot}%{_datadir}/%{name}/examples -name '*.so' -print -delete > > or just > > rm %{buildroot}%{_datadir}/%{name}/examples/plugin/hdTiny.so > > to remove > > /usr/share/usd/examples/plugin/hdTiny.so > > because there should not be arch-specific code in /usr/share. Done > > - Please fix mixed tabs and spaces in spec file, e.g. > > sed -r -i 's/\t/ /g' usd.spec Done. Thanks for the tip. Here is the updated SPEC: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-rawhide-x86_64/02130525-usd/usd.spec SRPM: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-rawhide-x86_64/02130525-usd/usd-21.02-4.fc35.src.rpm Some known issues are the placement of files within libs and devel. I would like a guide for those. Thanks. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
