[Bug 2085444] Review Request: sgx-sdk - Software Guard eXtension software development kit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=2085444



--- Comment #65 from Daniel Berrangé <berrange@xxxxxxxxxx> ---
> The source package can be named sgxsdk but I think the main binary package
> should be sgxsdk-devel. That's the naming used in similar cases when
> the package only ships dev content and not any shared libraries.
> examples: header only c++ libraries, rust crates

This requested change was not addressed.


> I understand libraries need to be bundled. Fedora packaging guidelines
> say they should be documented in that case, with Provides: lines in
> the spec file. See this section:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling
>
> Here's a couple starting points:
>
> Provides: bundled(protobuf) = 3.20.1
> Provides: bundled(tinyxml) = 7.0.0

This requested change was not addressed

> Here's some small changes: typo fix, trailing whitespace, use %autosetup:

This requested change was not addressed.


Please do not simply ignore requested changes from reviewers. It is is fine to
disagree with requests, but if you're not going to address something please
explain why the request was inappropriate.



> 5. Removed subpackage "sgxsdk-samples" stuff, which will be added later after sgx-aesm-service available Fedora.

I'm not happy with that. As mentioned in
https://bugzilla.redhat.com/show_bug.cgi?id=2030595#c36 since they are mutually
dependant, 
I think sgx-sdk and sgx-aesm-service need to be proposed together as a pair and
reviewed at the same time.

I don't think we should be approving this with bits cut out, which will be
re-added just after acceptance, but without review.


> # The entire source code is BSD, except some third party projects are
> # under other licenses listed in License.txt.
> License:        BSD and MIT and ASL 2.0 and NCSA/MIT and CC0 and FBSDDL and OpenSSL and zlib and GPL and BSD/GPLv2 and EPL-1.0

Fedora recently switched to using SPDX expressions for license declarations, so
I'm afraid this needs to be updated to follow that new standard

  https://docs.fedoraproject.org/en-US/legal/license-field/

NB, there is no license minimization done anymore - any license present in a
compiled source file needs to be listed - IOW, all versions of the GPL that are
present in source need explicitly listing

> # To build SGX SDK from linux-sgx source, first download the prebuilt
> # binaries by "make preparation", then run script
> # ./linux/installer/rpm/sdk/build.sh to update spec and repack tarball.
> # The repacked tarball is shared on 01.org for generating target RPMs.
> Source0:        https://download.01.org/intel-sgx/sgx_repo/rpm_onespec/%{name}-%{version}.tar.gz

These instructions don't appear to work when I try them

$ git clone https://github.com/intel/linux-sgx
$ cd linux-sgx
$ make preparation
...snip...
$ ./linux/installer/rpm/sdk/build.sh 
~/tmp/linux-sgx/linux/installer/rpm/sdk/sgxsdk-2.20.100.4 ~/tmp/linux-sgx
~/tmp/linux-sgx
readelf:
./linux/installer/rpm/sdk/../../../..//linux/installer/common/sdk/../../../..//build/linux/sgx_sign:
Error: No such file
./linux/installer/rpm/sdk/../../../..//linux/installer/common/sdk/createTarball.sh:
line 51: test: =: unary operator expected
Create the dest dir
Error !!!: src file not exist
/home/berrange/tmp/linux-sgx/build/linux/libsample_libcrypto.so

Seems like there is some intermediate step missing

> %autosetup -c

This is the correct thing to do in Fedora context, however, I would also
encourage filing a bug against the upstream project. It is really bad practice
for a release tarball to not unpack into a top level directory matching the
name+version of the tarball.

> %{_libdir}/*.so
> %{_libdir}/pkgconfig/*.pc
> %{_libdir}/sgx-gdb-plugin/
> %{_prefix}/lib/sgxsdk/
> %{_datadir}/sgxsdk/

I'd like to see all of these expand the list of files being packaged. This
serves two purposes - it causes a build failure if a file unexpectedly goes
missing  on a rebase, and it identifies when new stuff is added on a rebase.
Given that this package is bundling so much 3rd party code, I think the
identifying newly added libraries is especially important as it would likely
have implications for the license declaration.

> %{_includedir}/sgxsdk/

I don't want each .h file listed separately, but I'd like to see this list the
top level as again this clearly identifies what is being bundled:

%dir %{_includedir}/sgxsdk/
%{_includedir}/sgxsdk/sgx_*.h
%{_includedir}/sgxsdk/stdc++/
%{_includedir}/sgxsdk/libcxx/
%{_includedir}/sgxsdk/ipp/
%{_includedir}/sgxsdk/tlibc/
%{_includedir}/sgxsdk/tprotobuf/


-- 
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=2085444

Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202085444%23c65
_______________________________________________
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, report it: https://pagure.io/fedora-infrastructure/new_issue




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux