https://bugzilla.redhat.com/show_bug.cgi?id=1895567 Luya Tshimbalanga <luya_tfz@xxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(luya_tfz@thefinal | |zone.net) | --- Comment #20 from Luya Tshimbalanga <luya_tfz@xxxxxxxxxxxxxxxx> --- (In reply to Ben Beasley from comment #19) > This is getting much, much closer. There are still a few remaining things > that > need to be looked at. > Thanks for taking the time addrssing some issues. > > Issues: > ======= > - 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 > See: https://docs.fedoraproject.org/en-US/packaging- > guidelines/#_devel_packages > > This is spurious; these are documentation. If you ever manage to build the > documentation, you will need a -doc subpackage, and they should go in that. > For now, they are fine where they are. Sounds good. > > - Development (unversioned) .so files in -devel subpackage, if present. > Note: Unversioned so-files directly in %_libdir. > See: https://docs.fedoraproject.org/en-US/packaging- > guidelines/#_devel_packages Resolved. > > You have patched the CMake files to set an so-version, and offered the > change > upstream—great! But your change only works when PXR_BUILD_MONOLITHIC is > set, so > you need to set it. > > Then you can do something like: > > and > > %files libs > […] > %{_libdir}/lib%{name}_ms.so.%{libmajor} > […] > > and > > %files devel > […] > %{_libdir}/lib%{name}_ms.so > […] > > Interestingly, this also fixes the cmake install path, so you can remove > these lines: > > # Relocate cmake folder > mkdir -p %{buildroot}%{_libdir}/cmake > mv %{buildroot}%{_prefix}/cmake/* %{buildroot}%{_libdir}/cmake > rm -rf %{buildroot}%{_prefix}/cmake > > Note that the monolithic build means there is only one shared library file, > for better or for worse. Done. PXR_BUILD_MONOLITH is enabled and the above lines are removed. > - The License field appears correct, but you must break down which files are > covered by which licenses in a spec file comment or packaged auxiliary > license file, as mandated in > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#_multiple_licensing_scenarios. > > You can use “licensecheck -r” to help you look for files with various > licenses. > I realized I forgot to add with licenses are associated to the files. Using the above command reveals only ilmbase, pxrDouble conversion and lz4 are under BSD while the majority are under ASL 2.0 and remainig generated. > - The -devel package should Require: cmake-filesystem for %{_libdir}/cmake. > > - The package fails to own the following directories: > > /usr/lib64/usd/hdx, /usr/lib64/usd/ndr, /usr/lib64/usd/plugin, > /usr/lib64/usd/usd/resources, /usr/lib64/usd/usd, > /usr/lib64/usd/usd/resources/usd, /usr/lib64/usd/hd, /usr/lib64/usd/hdSt, > /usr/lib64/usd/hgiGL, /usr/lib64/usd/hio, /usr/lib64/usd/ar, > /usr/lib64/usd/glf > > The easiest way to fix this would be to change > > %dir %{_libdir}/%{name} > %{_libdir}/%{name}/sdf > %{_libdir}/%{name}/plugInfo.json > %{_libdir}/%{name}/{ar,glf,hd,hdSt,hdx,hgiGL,hio,ndr}/* > %{_libdir}/%{name}/plugin/* > %{_libdir}/%{name}/%{name}/resources/generatedSchema.usda > %{_libdir}/%{name}/%{name}/resources/plugInfo.json > %{_libdir}/%{name}/%{name}/resources/usd/schema.usda > > %{_libdir}/%{name}/%{name}{Geom,Hydra,Imaging,ImagingGL,Lux,Media,Render}/ > %{_libdir}/%{name}/%{name}{Ri,RiImaging,Shade,Skel,SkelImaging,UI}/ > %{_libdir}/%{name}/%{name}{Vol,VolImaging}/ > > to > > %{_libdir}/%{name} > %exclude %{_libdir}/%{name}/%{name}/resources/codegenTemplates > > - Version 21.05 is available. Please update. > Package is updated to 21.05 and the remaing lines are trimmed to the above suggestion. cmake-filesystem is added on devel subpackage. > - The error you reported building the Python components was because uic-qt5 > defaults to C++ output. You can fix it by adding, at the beginning of > %build: > > cat > uic-wrapper <<'EOF' > #!/bin/sh > exec uic-qt5 -g python "$@" > EOF > chmod +x uic-wrapper > > and then, in the arguments to %cmake, changing PYSIDEUICBINARY to: > > -DPYSIDEUICBINARY:PATH=${PWD}/uic-wrapper > Done. > > Now you have the problem that the package is installed under > %{python3_sitelib} even though it has compiled extensions. The ugly but > simple solution is to add this after %cmake_install: > > mkdir -p %{buildroot}%{python3_sitearch} > mv %{buildroot}%{python3_sitelib}/* %{buildroot}%{python3_sitearch} > > Worse, some files are installed under a flagrantly wrong path: > > mv %{buildroot}%{prefix}/lib/python/pxr/*.* \ > %{buildroot}%{python3_sitearch}/pxr/ > mv %{buildroot}%{prefix}/lib/python/pxr/Usdviewq/* \ > %{buildroot}%{python3_sitearch}/pxr/Usdviewq/ > > Instead of all those “mv” commands in %install, you might prefer instead to > patch the build system with sed in %prep; I have not taken the time to > figure > out the right way to do this. > > See the ancient issue > https://github.com/PixarAnimationStudios/USD/issues/30. The ugle workaround appears resolving the issue. The above command has a syntax releated to %{_prefix} but fixed on the incoming spec file. > > - It turns out upstream also bundles a couple of fonts. You can unbundle > these > by, in %prep: > > rm -rvf pxr/usdImaging/usdviewq/fonts/* > ln -s %{_datadir}/fonts/google-roboto > pxr/usdImaging/usdviewq/fonts/Roboto > ln -s %{_datadir}/fonts/google-roboto-mono \ > pxr/usdImaging/usdviewq/fonts/Roboto_Mono > > and add: > > Requires: font(roboto) > Requires: font(robotoblack) > Requires: font(robotolight) > Requires: font(robotomono) > > to the Python subpackage. See > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_avoid_bundling_of_fonts_in_other_packages. Done. > > - Once I got the Python components building, I tried: > > python3 -m pxr.Usdviewq > > and got (on Fedora 33): > > File "/usr/lib64/python3.9/runpy.py", line 188, in _run_module_as_main > mod_name, mod_spec, code = _get_module_details(mod_name, _Error) > File "/usr/lib64/python3.9/runpy.py", line 147, in _get_module_details > return _get_module_details(pkg_main_name, error) > File "/usr/lib64/python3.9/runpy.py", line 111, in _get_module_details > __import__(pkg_name) > File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/__init__.py", > line 30, in <module> > from .common import Timer > File > "/usr/lib64/python3.9/site-packages/shiboken2/files.dir/shibokensupport/ > __feature__.py", line 142, in _import > return original_import(name, *args, **kwargs) > File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/common.py", line > 28, in <module> > from pxr import Ar, Tf, Sdf, Kind, Usd, UsdGeom, UsdShade > File > "/usr/lib64/python3.9/site-packages/shiboken2/files.dir/shibokensupport/ > __feature__.py", line 142, in _import > return original_import(name, *args, **kwargs) > File "/usr/lib64/python3.9/site-packages/pxr/Ar/__init__.py", line 24, > in <module> > from . import _ar > File > "/usr/lib64/python3.9/site-packages/shiboken2/files.dir/shibokensupport/ > __feature__.py", line 142, in _import > return original_import(name, *args, **kwargs) > ImportError: /lib64/libjemalloc.so.2: cannot allocate memory in static > TLS block > > so I tried putting > > BuildRequires: pkgconfig(jemalloc) > > and > > -DPXR_MALLOC_LIBRARY="%{_libdir}/libjemalloc.so" \ > > behind a disabled build conditional, and got this instead: > > /usr/bin/python3: No module named pxr.Usdviewq.__main__; 'pxr.Usdviewq' > is a package and cannot be directly executed > > OK, I guess this isn’t how to start usdviewq, but the import is working, so > it looks like not using jemalloc is the way to go. Maybe there is some > other > way to fix this, but this way works—maybe at some performance cost, or > maybe > not. > jemalloc is disabled. Trying to execute usdview generates the following traceback: usdview Traceback (most recent call last): File "/usr/bin/usdview", line 28, in <module> import pxr.Usdviewq as Usdviewq File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/__init__.py", line 29, in <module> from .qt import QtWidgets, QtCore File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/qt.py", line 42, in <module> PySideModule = GetPySideModule() File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/qt.py", line 31, in GetPySideModule from . import attributeValueEditorUI File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/attributeValueEditorUI.py", line 11, in <module> from PySide2.QtCore import * > - It turns out that with Python enabled, you get quite a few more binaries in > the main “usd” package: > > /usr/bin/sdfdump: ELF 64-bit LSB pie executable, x86-64, version > 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, > BuildID[sha1]=d5c4551cc2fc937008004724d7dabeab13f92be2, for GNU/Linux 3.2.0, > stripped > /usr/bin/sdffilter: ELF 64-bit LSB pie executable, x86-64, version > 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, > BuildID[sha1]=f9c69423f6eea1aa506e58c95f99db631cfe78ec, for GNU/Linux 3.2.0, > stripped > /usr/bin/testusdview: Python script, ASCII text executable > /usr/bin/usdcat: Python script, ASCII text executable > /usr/bin/usdchecker: Python script, ASCII text executable > /usr/bin/usddiff: Python script, ASCII text executable > /usr/bin/usddumpcrate: Python script, ASCII text executable > /usr/bin/usdedit: Python script, ASCII text executable > /usr/bin/usdGenSchema: Python script, ASCII text executable > /usr/bin/usdrecord: Python script, ASCII text executable > /usr/bin/usdresolve: Python script, ASCII text executable > /usr/bin/usdstitch: Python script, ASCII text executable > /usr/bin/usdstitchclips: Python script, ASCII text executable > /usr/bin/usdtree: Python script, ASCII text executable > /usr/bin/usdview: Python script, ASCII text executable > /usr/bin/usdzip: Python script, ASCII text executable > > Hey, look, there’s the usdview launcher. Anyway, it looks like you now need > to either put most of these in the python3-usd subpackage, or (probably > better) you need to add > > Requires: python3-%{name}%{?_isa} = %{version}-%{release} > > to the main “usd” package. Done. > > - Since the importable Python package is actually called “pxr”, you should > add > > %py_provides python3-pxr > > per > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ > #_the_py_provides_macro > to get the right virtual provides. Or, you could rename the Python > subpackage > to match the Python import, and the %py_provides would be unnecessary. > > - I tried usdview on the file top.geom.usd and had the window appear for a > couple of seconds, with a glitchy, transparent hole where the OpenGL render > should be. Then it crashed: > > QSocketNotifier: Can only be used with threads started with QThread > State file not found, a new one will be created. > Traceback (most recent call last): > File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/stageView.py", > line 1659, in paintGL > renderer = self._getRenderer() > File "/usr/lib64/python3.9/site-packages/pxr/Usdviewq/stageView.py", > line 970, in _getRenderer > self._renderer = UsdImagingGL.Engine() > pxr.Tf.ErrorException: > Error in 'pxrInternal_v0_21__pxrReserved__::GlfContextCaps::_LoadCaps' > at line 162 in file > /builddir/build/BUILD/USD-21.02/pxr/imaging/glf/contextCaps.cpp : 'Failed > verification: ' GlfGLContext::GetCurrentGLContext()->IsValid() '' > Error in > 'pxrInternal_v0_21__pxrReserved__::{anonymous}::_IsHydraEnabled' at line 109 > in file > /builddir/build/BUILD/USD-21.02/pxr/usdImaging/usdImagingGL/engine.cpp : > 'OpenGL context required, using reference renderer' > /usr/include/c++/10/bits/stl_vector.h:1063: std::vector<_Tp, > _Alloc>::const_reference std::vector<_Tp, > _Alloc>::operator[](std::vector<_Tp, _Alloc>::size_type) const [with _Tp = > float; _Alloc = std::allocator<float>; std::vector<_Tp, > _Alloc>::const_reference = const float&; std::vector<_Tp, _Alloc>::size_type > = long unsigned int]: Assertion '__builtin_expect(__n < this->size(), true)' > failed. > Aborted (core dumped) > > Is this what happens for you, once you fix the Python build? > See above. For some reason, pyside2 is missing. > It looks similar to > https://github.com/PixarAnimationStudios/USD/issues/1467 > and https://github.com/PixarAnimationStudios/USD/issues/764, but not > identical. It may be something you can only report upstream. It would be > nice > not to have such a glaring bug right out of the gate, but I wouldn’t > consider > it a blocker for the package given how much other functionality it offers. > > - Since usdview is a desktop application, you will need to write a downstream > desktop file (which you can offer upstream): > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files. > > AppStream XML files, > https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/, are > good > too, and are a SHOULD, but I have found they are very hard to properly > create > downstream since they need to have screenshots and a description under an > ultra-permissive license such as CC0. The .desktop file, though, is > necessary. > Done. Appstream files is not included yet. Maybe once usdview is functional, I will take some screenshots and submit to upstream. For the desktop icon, upstream has logo but in a lower resolution. > - I think > > %global test OFF > > should be a build conditional: > > %bcond_with test > > and then you can replace > > -DPXR_BUILD_TESTS=%{test} \ > > with > > -DPXR_BUILD_TESTS=%{?with_test:ON}%{!?with_test:OFF} \ Done. > > - I would still like to see the tests runnable, but I am going to go ahead > and > provide this review while I look into what that would require. I intend to > offer suggestions here if I can figure out why they did not work for you. Thanks. > > Notes (a change is not required) > ================================ > > - This is not needed if you are only targeting F33 and later: > > %undefine __cmake_in_source_build > > (Since F32 is almost EOL, why bother with it?) You are right. The above line is now dropped. > > - You could drop the license file from the main package “usd” since it > depends > on “usd-libs”, which does have the license file. > Done. > - You have correctly justified the ExclusiveArch in a comment. After the > package is imported to dist-git, you must file Bugzilla tickets blocking > the > tracker bugs for all unsupported architectures, as required by > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_architecture_build_failures. > Will do. > - Instead of > > # Fix all Python shebangs recursively in . > pathfix.py -pni "%{__python3} %{py3_shbang_opts}" . > > it would be simpler to write > > %py3_shebang_fix . At last a macro. Fixed. Here is the updated SPEC: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-rawhide-x86_64/02155514-usd/usd.spec SRPM: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-rawhide-x86_64/02155514-usd/usd-21.05-1.fc35.src.rpm -- 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
