https://bugzilla.redhat.com/show_bug.cgi?id=1895567
--- Comment #15 from Luya Tshimbalanga <luya_tfz(a)thefinalzone.net> ---
(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.
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/fedor...
SRPM:
https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedor...
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