https://bugzilla.redhat.com/show_bug.cgi?id=2030595 Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@xxxxxxxxx --- Comment #21 from Zbigniew Jędrzejewski-Szmek <zbyszek@xxxxxxxxx> --- > %define _aesm_service_path /opt/intel/sgx-aesm-service > %define _ra_service_path /opt/intel/sgx-ra-service > %define _dcap_pccs_path /opt/intel/sgx-dcap-pccs > %define _psw_version 2.15.100.0 > %define _dcap_version 1.12.100.0 > %define _license_file COPYING Those defines are very suspicious. They should at least use %global: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_global_preferred_over_define %_psw_version is just the package version? Please drop the define, put the version in Version:, drop redundant Version lines on the subpackages. What are those paths? Files that are part of the package or some external stuff? If those files are part of the package, they should be in %_libdir/%name/ or %_libexedir/%name/. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_filesystem_layout What is the point of %_license_file? Just put "COPYING" directly in the relevant places. It's shorter and obvious. > Release: 1%{?dist} I'd recommend %autorelease+%autochangelog: https://docs.pagure.org/fedora-infra.rpmautospec/autochangelog.html > %description > Intel(R) Software Guard Extensions AESM Service The description should *describe*: what is this package good for, what it does, etc. Usually this is at least a paragraph of text. For complicated packages, maybe a few paragraphs. %description should also be wrapped to 80 columns. > Intel(R) Software Guard Extensions QE and PvE > Intel(R) Software Guard Extensions LE > ... Those summaries generally just repeat the package name, but don't say much about the package. In those cases, "LE" and "QE" and "PvE" are the interesting parts, but after reading the Summary, I have no idea what that is. > Requires: %{name} >= %{version}-%{release} libsgx-qe3-logic >= %{_dcap_version}-%{release} libsgx-aesm-pce-plugin >= %{version}-%{release} One per line please > Requires: gcc gcc-c++ make One per line. Hmm, why does package require a compiler at runtime? > make %{?_smp_mflags} build → %make_build > make DESTDIR=%{?buildroot} install → %make_install > if [ -c /dev/sgx_provision -o -c /dev/sgx/provision ] Generally, you need to assume that the rpm may be installed on a different machine (e.g. when we're creating a "golden image" for installation on other machines), or the rpm may installed into a chroot (e.g. when doing dnf install --sysroot=…). So conditionalization on /dev/sgx_provision being there is not possible. The group should be added unconditionally. A sysusers file should be used to declare the group and create a scriptlet automatically. See https://docs.fedoraproject.org/en-US/packaging-guidelines/UsersAndGroups/#_dynamic_allocation > %post > if [ -x %{_aesm_service_path}/startup.sh ]; then %{_aesm_service_path}/startup.sh; fi > %preun > if [ -x %{_aesm_service_path}/cleanup.sh ]; then %{_aesm_service_path}/cleanup.sh; fi > %post -n sgx-dcap-pccs > if [ -x %{_dcap_pccs_path}/startup.sh ]; then %{_dcap_pccs_path}/startup.sh; fi > %preun -n sgx-dcap-pccs > if [ -x %{_dcap_pccs_path}/cleanup.sh ]; then %{_dcap_pccs_path}/cleanup.sh; fi > %post -n sgx-ra-service > if [ -x %{_ra_service_path}/startup.sh ]; then %{_ra_service_path}/startup.sh; fi > %preun -n sgx-ra-service > if [ -x %{_ra_service_path}/cleanup.sh ]; then %{_ra_service_path}/cleanup.sh; fi What are those files? Who provides them? What do they do? (Note that those scriptlets will fail the transaction if those programs fail. You should always use '|| :'. But I'm not sure if they should be called at all.) > trigger_udev() { systemd-udev already does 'udevadm control --reload' from a transfiletrigger. But it doesn't do 'udevadm trigger'. Maybe it should. I'll discuss this with other maintainers of systemd and return to this later. Why is %install so complicated? %{buildroot} should not be conditionalized with ?. It's always defined. > find license -type f -print0 | \ > xargs -0 -n1 cat >> %{?buildroot}/${pkg}/%{_docdir}/${pkg}/%{_license_file} This merges multiple files into one file? Please don't do this. Just list the license files with %license, they will be copied automatically. (https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text) > find %{?buildroot}/${pkg} -type d -links 2 | \ > sed -e "s#^%{?buildroot}/${pkg}##" | So, you want to have a file list without the prefix? Just do "(cd %{buildroot}/$pkg; find * ...)". Also, %_specdir shouldn't be used to write anything. %buildsubdir is better. In general this automatic generation of file lists makes it very hard to see what files are where. Is it really necessary? I tried to build the package, but that fails, so I can't even figure out what files the package has :( -- inlined from 'void Json::Value::swapPayload(Json::Value&)' at /builddir/build/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/third_party/jsoncpp.cpp:2678:12, inlined from 'bool Json::OurReader::readValue()' at /builddir/build/BUILD/sgx-aesm-service-2.15.100.0/external/CppMicroServices/third_party/jsoncpp.cpp:1228:31: /usr/include/c++/12/bits/move.h:205:7: error: 'v.Json::Value::value_' may be used uninitialized [-Werror=maybe-uninitialized] 205 | __a = _GLIBCXX_MOVE(__b); | ^~~ Using -Werror for compiler heuristics is generally a bad idea. It means that the package may build fine one day, but fail to build the next, after the compiler has been upgraded. -- I didn't look at the contents yet. I think the most important part would be to improve the Summaries and %descriptions, and obviously to get the package to build again. I expect that there'll be a lot of work on unbundling and file locations… -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2030595 _______________________________________________ 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
