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 #25 from Luya Tshimbalanga <luya_tfz@xxxxxxxxxxxxxxxx> --- (In reply to Ben Beasley from comment #24) > 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. > > - Package contains BR: python2-devel or python3-devel > > Here, fedora-review is saying the mentioned BR is missing, because it > doesn’t > recognize > > BuildRequires: pkgconfig(python3) > > as equivalent to > > BuildRequires: python3-devel > > Since this will probably always work, I think it is permissible. However, > it’s still an odd way to express the dependency unless Python 3 is actually > detected using pkg-config/pkgconf. > It seemms like a bug for fedora-review because Python 3 is detected using pkgconfig(python3). See https://fedora.pkgs.org/rawhide/fedora-x86_64/python3-devel-3.9.5-1.fc35.x86_64.rpm.html listing pkgconfig files. > - Package does not contain duplicates in %files. > Note: warning: File listed twice: /usr/lib64/usd > > Since you have > %{_libdir}/%{name} > > in %files now, you can drop > > %dir %{_libdir}/%{name} Done. > > - You still need to document the license breakdown in the spec file > > (https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#_multiple_licensing_scenarios). > > I pretty much tabulated this for you in the original review. > > I believe this would be one correct implementation: > > # The entire source is ASL 2.0 except: > # > # BSD: > # - pxr/base/gf/ilmbase_* > # - pxr/base/js/rapidjson/msinttypes/ > # - pxr/base/tf/pxrDoubleConversion/ > # - pxr/base/tf/pxrLZ4/ > # MIT: > # - pxr/imaging/garch/khrplatform.h > # - pxr/base/js/rapidjson/, except pxr/base/js/rapidjson/msinttypes/ > # - pxr/base/tf/pyLock.cpp (only some sections; most of the file is > # ASL 2.0) > # - third_party/renderman-23/plugin/rmanArgsParser/pugixml/ > # MIT or Unlicense: > # - pxr/imaging/hio/stb/ > # > # (Certain build system files are also under licenses other than ASL > 2.0, but > # do not contribute their license terms to the built RPMs.) > License: ASL 2.0 and BSD and MIT and (MIT or Unlicense) > > You should probably double-check my work. Done. I double-check the list with yours and it matches. > > - A minor quibble, but I think if the usdview binary is going to be in the > “usd” package, the desktop file should be there too instead of in the > python3-usd package, i.e.: > > %files > %doc NOTICE.txt README.md > %{_bindir}/* > %dir %{_datadir}/%{name} > %dir %{_datadir}/%{name}/examples/ > %{_datadir}/%{name}/examples/* > %if %{with python3} > %{_datadir}/applications/org.open%{name}.%{name}view.desktop > %endif > > But in the end it’s going to work well enough either way. Maybe improve those lines after package is pushed on the main repository. > > - Rpmlint reports: > > python3-usd.x86_64: E: non-executable-script > /usr/lib64/python3.9/site-packages/pxr/Sdr/shaderParserTestUtils.py 644 > /usr/bin/python3 -s > python3-usd.x86_64: E: non-executable-script > /usr/lib64/python3.9/site-packages/pxr/UsdUtils/updateSchemaWithSdrNode.py > 644 /usr/bin/python3 -s > python3-usd.x86_64: E: non-executable-script > /usr/lib64/python3.9/site-packages/pxr/Usdviewq/usdviewApi.py 644 > /usr/bin/python3 -s > > and in fact, based on their contents, none of these would be usable as a > script even if they were executable, so you should remove the shebang > lines. > This would do it in %prep: > > sed -r -i '1{/^#!/d}' \ > pxr/usd/sdr/shaderParserTestUtils.py \ > pxr/usd/usdUtils/updateSchemaWithSdrNode.py \ > pxr/usdImaging/usdviewq/usdviewApi.py Done, > > - You reported: > > 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 * > > I see: > > python3-usd (rpmlib, GLIBC filtered): > font(roboto) > font(robotoblack) > font(robotolight) > font(robotomono) > ld-linux-x86-64.so.2()(64bit) > libOpenImageIO_Util.so.2.2()(64bit) > libboost_python39.so.1.75.0()(64bit) > libc.so.6()(64bit) > libgcc_s.so.1()(64bit) > libgcc_s.so.1(GCC_3.0)(64bit) > libgcc_s.so.1(GCC_3.3.1)(64bit) > libm.so.6()(64bit) > libpthread.so.0()(64bit) > libpython3.9.so.1.0()(64bit) > libstdc++.so.6()(64bit) > libstdc++.so.6(CXXABI_1.3)(64bit) > libstdc++.so.6(CXXABI_1.3.1)(64bit) > libstdc++.so.6(CXXABI_1.3.5)(64bit) > libstdc++.so.6(CXXABI_1.3.9)(64bit) > libtbb.so.2()(64bit) > libusd_ms.so.0()(64bit) > python(abi) > rtld(GNU_HASH) > > So the boost-python-based build is not generating the dist-info or egg-info > metadata that would be required for the Python dependency generator > > (https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ > #_automatically_generated_dependencies) > to work. You will have to manually add the appropriate dependencies (ones > used only as Python imports; those linked as C or C++ libraries are still > detectable) to the spec file: > > Requires: python3dist(jinja2) > Requires: python3dist(pyside2) > Requires: python3dist(pyopengl) usdview works now with these added requirement. > > - The line > > %dir %{_datadir}/%{name} > > is not a correct path for %files docs. (It is not affecting anything > since you are not currently building the documentation.) Fixed just in case. > > - You could drop the desktop-file-validate invocation since you are > installing > the desktop file with desktop-file-install. (However, the double validation > doesn’t do any harm.) Let's keep it for new. > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_desktop_file_install_usage > > - If you enable the tests, here’s the error I see: > > /home/reviewer/rpmbuild/BUILD/USD-21.05/pxr/base/tf/testenv/weakPtr.cpp: > In function 'bool Test_TfCreateRefPtrFromProtectedWeakPtr()': > > /home/reviewer/rpmbuild/BUILD/USD-21.05/pxr/base/tf/testenv/weakPtr.cpp:338: > 27: error: 'sleep_for' is not a member of 'std::this_thread' > 338 | > std::this_thread::sleep_for(std::chrono::milliseconds(250)); > | ^~~~~~~~~ > gmake[2]: *** [pxr/base/tf/CMakeFiles/testTf.dir/build.make:644: > pxr/base/tf/CMakeFiles/testTf.dir/testenv/weakPtr.cpp.o] Error 1 > gmake[2]: *** Waiting for unfinished jobs.... > gmake[2]: Leaving directory > '/home/reviewer/rpmbuild/BUILD/USD-21.05/x86_64-redhat-linux-gnu' > gmake[1]: *** [CMakeFiles/Makefile2:2961: > pxr/base/tf/CMakeFiles/testTf.dir/all] Error 2 > gmake[1]: Leaving directory > '/home/reviewer/rpmbuild/BUILD/USD-21.05/x86_64-redhat-linux-gnu' > gmake: *** [Makefile:163: all] Error 2 > error: Bad exit status from /var/tmp/rpm-tmp.f9uxSV (%build) > > It turns out that this is just a missing include, and there’s another one > you > run into after fixing that one. The patch usd-21.05-missing-includes.patc > fixes all of them; please forward it upstream. > > Now the tests are built and (unfortunately) installed, in the nonstandard > path /usr/tests/; cmake/macros/Public.cmake seems to govern this. There is > a > comment > > # XXX -- We shouldn't have to install to run tests. > > (I agree!) but I couldn’t find a corresponding upstream issue. There might > be > a way to change where the tests are installed—I didn’t look into it too > closely—or perhaps one could %exclude %{_prefix}/tests in %files, although > that use of %exclude (ignoring installed files altogether so that they are > not in any subpackage) is allegedly supposed to go away in a future version > of RPM. > > This > > %{?test:%ctest} > > must be now be changed to > > %{?with_test:%ctest} > > in order to try to run the tests. > > Once I do all that, I get a huge number of test failures because all the > tests are trying to use absolute paths, but they are installed inside the > buildroot directory. So I think the endgame here is an upstream issue for > the > patch I offered, plus an upstream issue about running the tests without > installing them, and comments linking those where the %check section would > be. I don’t think actually running the tests right now is going to be > practical. Right as test is disabled for the time being. > > - Man pages for the executables would be great, if practical. You might be > able > to use help2man to generate acceptable ones. If not, and if you don’t want > to > hand-write them, you can at least ask upstream if they would consider > providing them. See > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages. Filed requests upstream on https://github.com/PixarAnimationStudios/USD/issues/1518 Updated SPEC: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-34-x86_64/02173489-usd/usd.spec SRPM: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-34-x86_64/02173489-usd/usd-21.05-2.fc34.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
