https://bugzilla.redhat.com/show_bug.cgi?id=2030595
Zbigniew Jędrzejewski-Szmek <zbyszek(a)in.waw.pl> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |zbyszek(a)in.waw.pl
--- Comment #21 from Zbigniew Jędrzejewski-Szmek <zbyszek(a)in.waw.pl> ---
%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_prefer...
%_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_la...
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/...
%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/LicensingGuidel...)
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