Spec URL: https://yunyings.fedorapeople.org/sgx-sdk.spec SRPM URL: https://yunyings.fedorapeople.org/sgx-sdk-2.16.100.0-1.fc35.src.rpm Description: Intel Software Guard Extentions(SGX) software development kit. SGX SDK upstream: https://github.com/intel/linux-sgx Pre-processed SGX SDK tarball: https://download.01.org/intel-sgx/rpm_onespec/sgx-sdk-2.16.100.0.tar.gz Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=86988075 Fedora Account System Username: yunyings
I'm not taking this on as a formal review, but some low hanging fruit I noticed after a quick look at the spec & test build * "BuildRequires: perl" can be replaced with "perl-base" as I don't see a need for the historical full perl module set * The "%description" is terse to the say the least, just repeating the Summary - "Intel(R) SGX SDK" - this needs to be more verbose - a few sentences * The section find %{?buildroot}/license -type f -print0 | \ xargs -0 -n1 cat >> %{?buildroot}%{_docdir}/%{name}/COPYING rm -fr %{?buildroot}/license echo "%{_install_path}" > %{_specdir}/listfile find %{?buildroot} -type d -exec \ sh -c '(ls -p "{}"|grep />/dev/null)||echo "{}"' \; | \ sed -e "s#^%{?buildroot}##" | \ grep -v "^%{_libdir}" | \ grep -v "^%{_bindir}" | \ grep -v "^%{_install_path}" | \ sed -e "s#^#%dir #" >> %{_specdir}/listfile find %{?buildroot} -type f | \ sed -e "s#^%{?buildroot}##" | \ grep -v "^%{_install_path}" >> %{_specdir}/listfile %files -f %{_specdir}/listfile Is rather unpleasantly obscuring what is actually being packaged. With this kind of magic it is way too easy to accidentally ship undesirable files in the final RPM. IMHO the %file section should be listing stuff explicitly, with few wildcards, to make it clear what is being shipped. ie it is reasonable to wildcard header files *.h, but not wildcard the nested trees. As a case in point, this current magic obscures the fact that the majority of files in this RPM has been put into /opt/intel/sgxsdk. Fedora RPMs are not expected to use /opt except in rare cases https://docs.fedoraproject.org/en-US/packaging-guidelines/#_limited_usage_of_opt_etcopt_and_varopt so if there's a good reason for use of /opt it needs a justification to be provided * The license text is duplicated in two different locations /usr/share/doc/sgx-sdk/COPYING /usr/share/licenses/sgx-sdk/License.txt * The pkg-config files are corrupt - the start of all of them looks like /opt/intel/sgxsdk includedir=${prefix}/include libdir=${prefix}/lib64 I believe that first line is supposed to be 'prefix=/opt/intel/sgxsdk' * sgx-gdb is shipped twice in two different locations * The package possibly ought to have a -devel sub-RPM for the include files & .pc files, so they're separate from the runtime .so files * sample code would likely be better in a -samples sub-RPM since there's quite alot of it
(In reply to Daniel Berrangé from comment #1) > I'm not taking this on as a formal review, but some low hanging fruit I > noticed after a quick look at the spec & test build > > * "BuildRequires: perl" can be replaced with "perl-base" as I don't see a > need for the historical full perl module set > > * The "%description" is terse to the say the least, just repeating the > Summary - "Intel(R) SGX SDK" - this needs to be more verbose - a few > sentences > > * The section > > find %{?buildroot}/license -type f -print0 | \ > xargs -0 -n1 cat >> %{?buildroot}%{_docdir}/%{name}/COPYING > rm -fr %{?buildroot}/license > echo "%{_install_path}" > %{_specdir}/listfile > find %{?buildroot} -type d -exec \ > sh -c '(ls -p "{}"|grep />/dev/null)||echo "{}"' \; | \ > sed -e "s#^%{?buildroot}##" | \ > grep -v "^%{_libdir}" | \ > grep -v "^%{_bindir}" | \ > grep -v "^%{_install_path}" | \ > sed -e "s#^#%dir #" >> %{_specdir}/listfile > find %{?buildroot} -type f | \ > sed -e "s#^%{?buildroot}##" | \ > grep -v "^%{_install_path}" >> %{_specdir}/listfile > > %files -f %{_specdir}/listfile > > Is rather unpleasantly obscuring what is actually being packaged. With > this kind of magic it is way too easy to accidentally ship undesirable files > in the final RPM. > > IMHO the %file section should be listing stuff explicitly, with few > wildcards, to make it clear what is being shipped. ie it is reasonable to > wildcard header files *.h, but not wildcard the nested trees. > > As a case in point, this current magic obscures the fact that the majority > of files in this RPM has been put into /opt/intel/sgxsdk. Fedora RPMs are > not expected to use /opt except in rare cases > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_limited_usage_of_opt_etcopt_and_varopt > > so if there's a good reason for use of /opt it needs a justification to be > provided [Xiangquan] I would like to explain the reason why using lines of scripts here. We want to share some scripts/files between rpm and debian. In this case, we just need to update one place for both rpm and debian. For example, if one file is added or deleted, only BOM file is updated. If it is not acceptable, it will be updated. > > > * The license text is duplicated in two different locations > > /usr/share/doc/sgx-sdk/COPYING > /usr/share/licenses/sgx-sdk/License.txt > [Xiangquan] Will remove one of them. > > * The pkg-config files are corrupt - the start of all of them looks like > > /opt/intel/sgxsdk > includedir=${prefix}/include > libdir=${prefix}/lib64 > > I believe that first line is supposed to be 'prefix=/opt/intel/sgxsdk' > [Xiangquan] Will check it. > * sgx-gdb is shipped twice in two different locations > [Xiangquan] Will check it. > * The package possibly ought to have a -devel sub-RPM for the include files > & .pc files, so they're separate from the runtime .so files > [Xiangquan] Actually SGXSDK is package just for developers. > * sample code would likely be better in a -samples sub-RPM since there's > quite alot of it [Xiangquan] Actually SGXSDK is package just for developers.
Per Fedora guidelines, we can not use /opt in general, you could technically request a FPC exception for /etc/fedora sub-directory: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_limited_usage_of_opt_etcopt_and_varopt The "Open Source" license mentioned in the License tag isn't in: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Software_License_List I can confirm that the script in %install section is pretty complicated, having a list of wildcards in %files would sound much safer. > Actually SGXSDK is package just for developers. But several files are installed into the default library path /usr/lib64, unversioned. These are typically needed at runtime, or should these be installed elsewhere? Typically, /usr/lib64 is for users (runtime), not developers (build time). /usr/lib64/libsgx_quote_ex_sim.so /usr/lib64/libsgx_launch_sim.so /usr/lib64/libsgx_epid_sim.so /usr/lib64/libsgx_urts_sim.so /usr/lib64/libsgx_uae_service_sim.so
(In reply to xiangquan.liu from comment #2) > (In reply to Daniel Berrangé from comment #1) > > * The section > > > > find %{?buildroot}/license -type f -print0 | \ > > xargs -0 -n1 cat >> %{?buildroot}%{_docdir}/%{name}/COPYING > > rm -fr %{?buildroot}/license > > echo "%{_install_path}" > %{_specdir}/listfile > > find %{?buildroot} -type d -exec \ > > sh -c '(ls -p "{}"|grep />/dev/null)||echo "{}"' \; | \ > > sed -e "s#^%{?buildroot}##" | \ > > grep -v "^%{_libdir}" | \ > > grep -v "^%{_bindir}" | \ > > grep -v "^%{_install_path}" | \ > > sed -e "s#^#%dir #" >> %{_specdir}/listfile > > find %{?buildroot} -type f | \ > > sed -e "s#^%{?buildroot}##" | \ > > grep -v "^%{_install_path}" >> %{_specdir}/listfile > > > > %files -f %{_specdir}/listfile > > > > Is rather unpleasantly obscuring what is actually being packaged. With > > this kind of magic it is way too easy to accidentally ship undesirable files > > in the final RPM. > > > > IMHO the %file section should be listing stuff explicitly, with few > > wildcards, to make it clear what is being shipped. ie it is reasonable to > > wildcard header files *.h, but not wildcard the nested trees. > > > > As a case in point, this current magic obscures the fact that the majority > > of files in this RPM has been put into /opt/intel/sgxsdk. Fedora RPMs are > > not expected to use /opt except in rare cases > > > > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > > #_limited_usage_of_opt_etcopt_and_varopt > > > > so if there's a good reason for use of /opt it needs a justification to be > > provided > > [Xiangquan] I would like to explain the reason why using lines of scripts > here. We want to share some scripts/files between rpm and debian. In this > case, we just need to update one place for both rpm and debian. For example, > if one file is added or deleted, only BOM file is updated. If it is not > acceptable, it will be updated. If an official Debian package is ever to be added to the main Debian package repos, it shouldn't use /opt either per their lint guidelines https://lintian.debian.org/tags/dir-or-file-in-opt In any case, other packaging choices made for distros are not a suitable justification to diverge from Fedora packaging guidelines excluding use of /opt > > * The package possibly ought to have a -devel sub-RPM for the include files > > & .pc files, so they're separate from the runtime .so files > > > > [Xiangquan] Actually SGXSDK is package just for developers. That's true of lots of packages, but generally if there's a packaging shipping ELF libraries, normal practice is for the header/pkg-config files which are needed ONLY at compile time, to be separated from the ELF files which are needed at compile and execution time. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages There can be exceptions, for example the above page notes "compilers often include development files in the main package because compilers are themselves only used for software development, thus, a split package model does not make any sense." but AFAICT, this sgx-sdk package doesn't contain compilers/toolchains, just a bunch of libraries & headers
Some other low-hanging fruit. Consider using the %make_build and %make_install macros at %build and %install sections respectively. Also better be more explicit in regards to the python dependency, it should be python3-devel.
(In reply to Pavel Raiskup from comment #3) > Per Fedora guidelines, we can not use /opt in general, you could technically > request a FPC exception for /etc/fedora sub-directory: > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_limited_usage_of_opt_etcopt_and_varopt > > The "Open Source" license mentioned in the License tag isn't in: > https://fedoraproject.org/wiki/Licensing: > Main?rd=Licensing#Software_License_List > > I can confirm that the script in %install section is pretty complicated, > having a list of wildcards in %files would sound much safer. > > > Actually SGXSDK is package just for developers. > > But several files are installed into the default library path /usr/lib64, > unversioned. These are typically needed at runtime, or should these be > installed elsewhere? Typically, /usr/lib64 is for users (runtime), not > developers (build time). > > /usr/lib64/libsgx_quote_ex_sim.so > /usr/lib64/libsgx_launch_sim.so > /usr/lib64/libsgx_epid_sim.so > /usr/lib64/libsgx_urts_sim.so > /usr/lib64/libsgx_uae_service_sim.so [Xiangquan] These dynamic libraries are only for simulation mode. In the real runtime environment, another set of dynamic libraries located in PSW/DCAP packages will be used.
Thanks all for your replies. Just want to know if all the SGX SDK files all installed into some directory under /opt/fedora, can we just put all the files into one package(means does not separate *.pc/*-sim.so, Sample)
(In reply to xiangquan.liu from comment #7) > Thanks all for your replies. > Just want to know if all the SGX SDK files all installed into some directory > under /opt/fedora, can we just put all the files into one package(means does > not separate *.pc/*-sim.so, Sample) Note even using /opt/fedora is NOT an option unless the FPC (Fedora Packaging Committee) explicitly grant an exception to permit that for SGX. IMHO, this should just using the normal filesystem locations not /opt at all.
(In reply to Daniel Berrangé from comment #8) > (In reply to xiangquan.liu from comment #7) > > Thanks all for your replies. > > Just want to know if all the SGX SDK files all installed into some directory > > under /opt/fedora, can we just put all the files into one package(means does > > not separate *.pc/*-sim.so, Sample) > > Note even using /opt/fedora is NOT an option unless the FPC (Fedora > Packaging Committee) explicitly grant an exception to permit that for SGX. > IMHO, this should just using the normal filesystem locations not /opt at all. Thanks for the quick reply. Do you any suggestions where to install SDK?
(In reply to xiangquan.liu from comment #9) > Thanks for the quick reply. Do you any suggestions where to install SDK? Normal practice is to honour the locations for each type of content that are set by RPM macros %{_prefix}, %{_exec_prefix}, %{_bindir}, %{_sbindir}, %{_sysconfdir}, %{_datadir}, %{_includedir}, %{_libdir}, %{_libexecdir}, %{_localstatedir}, %{_sharedstatedir}, %{_mandir}, %{_infodir}.
So is it OK if all the SDK files are installed under directory /usr/share? Ex. /usr/share/intel/sgxsdk?
(In reply to xiangquan.liu from comment #11) > So is it OK if all the SDK files are installed under directory /usr/share? > Ex. /usr/share/intel/sgxsdk? /usr/share is for architecture independent files, so that looks inappropriate for the SDK with many native binaries. Looking at what's in the RPM currently I'd think /opt/intel/sgxsdk/SampleCode would be %{_datadir}/sgx-sdk/SampleCode, /opt/intel/sgxsdk/include would be %{_includedir}/sgx-sdk. I'm still unclear what criteria was used to choose between /opt/intel/sgx-sdk/lib and /usr/lib64 in the current RPM, but assuming there's a reason that some libs needed to be in a different dir, then /opt/intel/sgxsdk/lib likely matches to a subdir like %{_libdir}/sgx-sdk (or possibly even %{_prefix}/lib/sgx-sdk, since multilib may well be irrelevant in the context of the SDK libs).
@xiangquan.liu, @yunying.sun do you have an updated SPEC file or SRPM you could share?
> do you have an updated SPEC file or SRPM you could share? Not yet, and sorry for the delayed update. Most issues pointed out in comments above need upstream changes. Xiangquan is working internally within the team to make those changes. Once upstream changes are finalized, I will update SPEC and SRPM asap and add comment here.
Thank you all for the reviews and comments. Updated Spec: https://yunyings.fedorapeople.org/sgx-sdk.spec Updated SRPM: https://yunyings.fedorapeople.org/sgx-sdk-2.16.100.0-1.fc35.src.rpm Updated Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=89480110 This update is trying to fix most of the issues mentioned in previous comments. Some exceptions: 1. "BuildRequires: perl" is not changed, since using "perl-base" instead leads to various build errors complaining not able to find many other perl libs like perl-FindBin/perl-lib(for lib.pm). 2. .pc and sample files are not separated to -devel and -sample subpackages. As Xiangquan said, the whole SDK is for development only, so we try to keep all within one package. Please let us know if subpackages are must-to-have. 3. After using "BuildRequires: python3-devel" instead of "python", compiling fails for can't find "python". New "BuildRequires: python-unversioned-command" is added to fix the failure.
This is an automatic action taken by review-stats script. The ticket reviewer failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we reset the status and the assignee of this ticket.
(Quoting Daniel Berrangé from comment #1) > Is rather unpleasantly obscuring what is actually being packaged. With > this kind of magic it is way too easy to accidentally ship undesirable files > in the final RPM. > > IMHO the %file section should be listing stuff explicitly, with few > wildcards, to make it clear what is being shipped. ie it is reasonable to > wildcard header files *.h, but not wildcard the nested trees. Please address this. (Quoting Daniel Berrangé from comment #12) > /usr/share is for architecture independent files, so that looks inappropriate for the SDK with many native binaries. What's your rationale for duplicating, for example, bin or include within /usr/share/sgx-sdk/? For example, what's stopping these from being in %{_bindir}? /usr/share/sgx-sdk/bin/x64/sgx_sign /usr/share/sgx-sdk/bin/x64/sgx_protoc /usr/share/sgx-sdk/bin/x64/sgx_edger8r /usr/share/sgx-sdk/bin/x64/sgx_config_cpusvn /usr/share/sgx-sdk/bin/x64/sgx_encrypt (Quoting Daniel Berrangé from comment #12) > Looking at what's in the RPM currently I'd think [...] /opt/intel/sgxsdk/include would be %{_includedir}/sgx-sdk. It's still in %{_datadir}/sgx-sdk/include. Is there any reason it can't be moved to %{_includedir}/sgx-sdk? (Quoting Daniel Berrangé from comment #12) >I'm still unclear what criteria was used to choose between /opt/intel/sgx-sdk/lib and /usr/lib64 in the current RPM, but assuming there's a reason that some libs needed to be in a different dir, then /opt/intel/sgxsdk/lib likely matches to a subdir like %{_libdir}/sgx-sdk (or possibly even %{_prefix}/lib/sgx-sdk, since multilib may well be irrelevant in the context of the SDK libs). It's still in %{_datadir}/sgx-sdk/lib64. Is there any reason it can't be moved to %{_libdir}/sgx-sdk or %{_prefix}/lib/sgx-sdk? (In reply to Yunying Sun from comment #15) > 1. "BuildRequires: perl" is not changed, since using "perl-base" instead > leads to various build errors complaining not able to find many other perl > libs like perl-FindBin/perl-lib(for lib.pm). As stated in Packaging Guidelines: Build-Time Dependencies (BuildRequires), a package must explicitly indicate its build dependencies (using BuildRequires:) outside of the minimal set required for RPM to build packages. This includes any dependency on Perl. While Perl may have been in the default buildroot at one time, this is not currently the case. Below is a list of Perl-related build dependencies you may need. perl-interpreter – The Perl interpreter must be listed as a build dependency if it is called in any way, either explicitly via perl or %__perl, or as part of your package’s build system. perl-devel - Provides Perl header files. If building architecture-specific code which links to libperl.so library (e.g. an XS Perl module), you must include BuildRequires: perl-devel. If a specific Perl module is required at build time, use +perl(+`+MODULE++)+`. This applies to so called core modules as well, since they may move in and out of the base Perl package over time. If you need to limit your package to a specific Perl version, use perl(:VERSION) dependency with desired version constraint (e.g. perl(:VERSION) >= 5.22). Do not use a comparison against the version of the perl package because it includes an epoch number, which makes version comparisons tricky. (In reply to Yunying Sun from comment #15) > 3. After using "BuildRequires: python3-devel" instead of "python", compiling > fails for can't find "python". New "BuildRequires: > python-unversioned-command" is added to fix the failure. You shouldn't depend on python-unversioned-command. Please look into %py3_shebang_fix.
It seems like there are some runtime dependencies that are not listed, e.g., gdb or python readelf module.
It's a big change for us. So we need to discuss these changes first and then give the feedback.
@Kalina, Thanks for the comments. According to the packaging-guidelines, we are not recommended to release static libraries. But trust enclave build depends on the static libraries which should be released in SGXSDK package. So the question is where to put these static libraries?
(In reply to xiangquan.liu from comment #21) > @Kalina, Thanks for the comments. > According to the packaging-guidelines, we are not recommended to release > static libraries. But trust enclave build depends on the static libraries > which should be released in SGXSDK package. So the question is where to put > these static libraries? /usr/lib(64) is usually for libraries that will be link to applications designed to run on the host OS. Since these libs are intended to instead run in the enclave, I think we can rule out use of /usr/lib(64) directly. As mentioned in earlier comments, something like %{_libdir}/sgx-sdk or %{_prefix}/lib/sgx-sdk would be a reasonable location
@Berrangé, thanks for the quick response. Do you have any recommendation for where to put these Sample Codes? And do we have to separate it into another package?
(In reply to Cestmir Kalina from comment #18) > (Quoting Daniel Berrangé from comment #1) To address all the issues mentioned in earlier comments, we have updated the spec and srpm: Updated SPEC: https://yunyings.fedorapeople.org/sgxsdk.spec Updated SRPM: https://yunyings.fedorapeople.org/sgxsdk-2.18.100.0-1.fc36.src.rpm Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=93622331 Could you help to review it again? Thanks.
The pre-processed tarball is available at: https://download.01.org/intel-sgx/rpm_onespec/sgxsdk-2.18.100.0.tar.gz .
I've been asked to provide some feedback on the specfile. > %undefine _auto_set_build_flags > %undefine __brp_mangle_shebangs This absolutely deserves an explanation in the spec file. Why is this needed? For the shebangs mangling, have you considered explicit opt-out for some files instead? IDeally, you want to get rid of this. > Summary: Intel(R) SGX SDK The guidelines say: "Never use (TM) or (R) (or the Unicode equivalents, ™/®). It is incredibly complicated to use these properly, so it is actually safer for us to not use them at all." See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_trademarks_in_summary_or_description > License: BSD and "Redistributable, no modification permitted" and MIT and ASL 2.0 and NCSA/MIT and CC0 and FBSDDL and BSD and OpenSSL and zlib and GPL and BSD/GPLv2 and EPL-1.0 Fedora has switched to SPDX license expression in the meantime. Please see https://docs.fedoraproject.org/en-US/legal/allowed-licenses/ The "Redistributable, no modification permitted" thing was never supposed to be in quotes in the first place, but with SPDX this might be hard to express. I recommend asking for help at the Fedora legal mailing list: https://lists.fedoraproject.org/admin/lists/legal.lists.fedoraproject.org/ > BuildRequires: redhat-rpm-config This BuildRequires is kinda weird. This package will always be present. I know that "explicit is better than implicit" but is there anything in particular you explicitly need from this package? > %description > The Intel(R) SGX SDK is a collection of APIs, libraries, documentation, sample source code, and tools that allows software developers to create and debug Intel(R) SGX enabled applications in C/C++. The same remark about (R) and also "please make sure that there are no lines in the description longer than 80 characters" https://docs.fedoraproject.org/en-US/packaging-guidelines/#_summary_and_description > Group: Development/Libraries "The Group: tag SHOULD NOT be used." https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections > Requires: %{name} = %{version}-%{release} libsgx-urts >= %{version}-%{release} libsgx-enclave-common >= %{version}-%{release} (personal opinion) This would have been much more readable if each fo the requirement was on a separate line / Requires: tag. > %description -n sgxsdk-samples > Intel(R) Software Guard Extensions SDK Sample Code for Developers The same remark about (R) and also please use sentences in the description (rpmlint will probably hint that this description does not end with "."). > %files > ... > %{_bindir}/* > %{_libdir}/*.so > %{_libdir}/pkgconfig/*.pc "Packagers SHOULD NOT simply glob everything under a shared directory." https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists > %{_includedir}/sgxsdk > %{_prefix}/lib/sgxsdk > %{_datadir}/sgxsdk (personal opinion) If those are directories, terminate the lines with a trailing slash please -- it makes it easier for the reader and serves as one additional layer of safety (the build would fail then if it is a regular file). > %changelog > %autochangelog I am not sure you can use %autochangelog without also using %autorelease in the Release tag.
(In reply to Miro Hrončok from comment #26) > I've been asked to provide some feedback on the specfile. > > > > > %undefine _auto_set_build_flags > > %undefine __brp_mangle_shebangs > > This absolutely deserves an explanation in the spec file. Why is this > needed? For the shebangs mangling, have you considered explicit opt-out for > some files instead? IDeally, you want to get rid of this. I would like to explain these two lines. %undefine _auto_set_build_flags - To avoid additional compile options from rpmbuild since it may cause some compile errors for the package build. %undefine __brp_mangle_shebangs - To avoid package build errors since these python scripts(gdb plugins) are using internal python interpreter. *** ERROR: ambiguous python shebang in /usr/lib64/sgx-gdb-plugin/gdb_sgx_plugin.py: #!/usr/bin/env python. Change it to python3 (or python2) explicitly. *** ERROR: ambiguous python shebang in /usr/lib64/sgx-gdb-plugin/load_symbol_cmd.py: #!/usr/bin/env python. Change it to python3 (or python2) explicitly. *** ERROR: ambiguous python shebang in /usr/lib64/sgx-gdb-plugin/readelf.py: #!/usr/bin/env python. Change it to python3 (or python2) explicitly. *** ERROR: ambiguous python shebang in /usr/lib64/sgx-gdb-plugin/sgx_emmt.py: #!/usr/bin/env python. Change it to python3 (or python2) explicitly.
(In reply to xiangquan.liu from comment #27) > (In reply to Miro Hrončok from comment #26) > > I've been asked to provide some feedback on the specfile. > > > > > > > > > %undefine _auto_set_build_flags > > > %undefine __brp_mangle_shebangs > > > > This absolutely deserves an explanation in the spec file. Why is this > > needed? For the shebangs mangling, have you considered explicit opt-out for > > some files instead? IDeally, you want to get rid of this. > > I would like to explain these two lines. > %undefine _auto_set_build_flags - To avoid additional compile options from > rpmbuild since it may cause some compile errors for the package build. What exact flags cause what exact errors? There is no "may cause some compile errors". Either there are errors or there are not not. If there are errors, either some specific flags need to be disabled/changed with a rationale (a rationale is not "it may error" but rather "flag XY cannot be used because the code does ABC in GHJ") or the errors need to be fixed. Simple disabling all flags entirely does not make sense. If there are no errors, simply disabling the flags as prevention makes no sense. Considering the spec file compiles stuff with %make_build I am not even sure disabling auto_set_build_flags actually makes a difference. > %undefine __brp_mangle_shebangs - To avoid package build errors since these > python scripts(gdb plugins) are using internal python interpreter. What does "internal python interpreter" mean? If the files are not executed in classic shell by their shebang, do they even need to have shebangs and be executable? If they absolutely need to have "#!/usr/bin/env python" shebang and be executable (which I am reluctant to belive): - exclude the specific directory/files from the shebang mangling, don't disable it: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shebang_lines - explain int the comment why this unusual and dangerous sehabng is actually needed to;dr Please try to avoid those lines as much as possible and if you cannot, explain why (int he spec), instead of explaining what those two lines do to me (I know what the lines do).
(In reply to Miro Hrončok from comment #28) > (In reply to xiangquan.liu from comment #27) > > (In reply to Miro Hrončok from comment #26) > > > I've been asked to provide some feedback on the specfile. > > > > > > > > > > > > > %undefine _auto_set_build_flags > > > > %undefine __brp_mangle_shebangs > > > > > > This absolutely deserves an explanation in the spec file. Why is this > > > needed? For the shebangs mangling, have you considered explicit opt-out for > > > some files instead? IDeally, you want to get rid of this. > > > > I would like to explain these two lines. > > %undefine _auto_set_build_flags - To avoid additional compile options from > > rpmbuild since it may cause some compile errors for the package build. > > What exact flags cause what exact errors? There is no "may cause some > compile errors". Either there are errors or there are not not. > > If there are errors, either some specific flags need to be disabled/changed > with a rationale (a rationale is not "it may error" but rather "flag XY > cannot be used because the code does ABC in GHJ") or the errors need to be > fixed. Simple disabling all flags entirely does not make sense. > > If there are no errors, simply disabling the flags as prevention makes no > sense. > > Considering the spec file compiles stuff with %make_build I am not even sure > disabling auto_set_build_flags actually makes a difference. I just don't understand why do we need to use the default compile/link options as below since we have our own compile/link options. Just want to make sure it is a must rule we need to follow? Thanks! $ rpmbuild --eval %set_build_flags CFLAGS="${CFLAGS:--O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection}" ; export CFLAGS ; CXXFLAGS="${CXXFLAGS:--O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection}" ; export CXXFLAGS ; FFLAGS="${FFLAGS:--O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -I/usr/lib64/gfortran/modules}" ; export FFLAGS ; FCFLAGS="${FCFLAGS:--O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -I/usr/lib64/gfortran/modules}" ; export FCFLAGS ; LDFLAGS="${LDFLAGS:--Wl,-z,relro -Wl,--as-needed -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld }" ; export LDFLAGS ; LT_SYS_LIBRARY_PATH="${LT_SYS_LIBRARY_PATH:-/usr/lib64:}" ; export LT_SYS_LIBRARY_PATH ; CC="${CC:-gcc}" ; export CC ; CXX="${CXX:-g++}" ; export CXX
> > %undefine __brp_mangle_shebangs - To avoid package build errors since these > > python scripts(gdb plugins) are using internal python interpreter. > > What does "internal python interpreter" mean? If the files are not executed > in classic shell by their shebang, do they even need to have shebangs and be > executable? > > If they absolutely need to have "#!/usr/bin/env python" shebang and be > executable (which I am reluctant to belive): > > - exclude the specific directory/files from the shebang mangling, don't > disable it: > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shebang_lines > - explain int the comment why this unusual and dangerous sehabng is > actually needed > We'll discuss this issue and try to fix it. Thanks.
> Just want to make sure it is a must rule we need to follow? Yes. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags """Compilers used to build packages must honor the applicable compiler flags set in the system rpm configuration.""" The documentation is at https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/buildflags.md As said, even without %_auto_set_build_flags this spec is likely already using the flags via %make_build anyway.
(In reply to Miro Hrončok from comment #26) > > License: BSD and "Redistributable, no modification permitted" and MIT and ASL 2.0 and NCSA/MIT and CC0 and FBSDDL and BSD and OpenSSL and zlib and GPL and BSD/GPLv2 and EPL-1.0 > > Fedora has switched to SPDX license expression in the meantime. Please see > https://docs.fedoraproject.org/en-US/legal/allowed-licenses/ > > The "Redistributable, no modification permitted" thing was never supposed to > be in quotes in the first place, but with SPDX this might be hard to > express. I recommend asking for help at the Fedora legal mailing list: > https://lists.fedoraproject.org/admin/lists/legal.lists.fedoraproject.org/ > Thank you Miro for the review. For the Intel signed binaries license issue, we are working internally with Intel legal folks to fix it. A thread for this license discussion in Fedora legal mail list: https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/W2FYSZ42RMX4IGHK3YNUXWBXUATSWTD6/
According to Intel, the Q1 release from last week includes the license update to 3BSD.
Yunying, are you going to submit a new version? Thanks!
*** Bug 1995371 has been marked as a duplicate of this bug. ***
*** Bug 1995373 has been marked as a duplicate of this bug. ***
*** Bug 1995376 has been marked as a duplicate of this bug. ***
*** Bug 1995375 has been marked as a duplicate of this bug. ***
Yes Paolo, I'm working with Xiangquan on updating the spec and srpm. Currently the new release tarball has some build errors both locally on my Fedora 37 and on koji, which seems introduced by removing the "%undefine _auto_set_build_flags" from spec. Xiangquan and Intel SGX team is checking. I will update here once new version is ready.
We are running into a compile issue which only exits in fedora rawhide environment with gcc13. This part is ported from standard libcxx, so we can't make any changes. Do you have any suggestions about this issue? Thanks! g++ -c -Wnon-virtual-dtor -std=c++17 -fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG -ffunction-sections -fdata-sections -Wall -Wextra -Winit-self -Wpointer-arith -Wreturn-type -Waddress -Wsequence-point -Wformat-security -Wmissing-include-dirs -Wfloat-equal -Wundef -Wshadow -Wcast-align -Wconversion -Wredundant-decls -DITT_ARCH_IA64 -fcf-protection -ffreestanding -nostdinc -fvisibility=hidden -fpie -fno-strict-overflow -fno-delete-null-pointer-checks -B/usr/local/bin -nostdinc++ -Werror -fno-rtti -fno-exceptions -I/builddir/build/BUILD/sgxsdk-2.19.100.0/common/inc/ -I/builddir/build/BUILD/sgxsdk-2.19.100.0/common/inc/internal/ -I/builddir/build/BUILD/sgxsdk-2.19.100.0/common/inc/tlibc -I/builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tseal -I/builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/selib -I/builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include ../sgx_secure_align.cpp -o ../sgx_secure_align.o make[3]: Leaving directory '/builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/selib/linux' In file included from /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/__tuple:15, from /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:199, from /builddir/build/BUILD/sgxsdk-2.19.100.0/common/inc/sgx_secure_align.h:37, from ../sgx_secure_align.cpp:34: /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1729:8: error: expected identifier before '__is_convertible' 1729 | struct __is_convertible | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1729:8: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1735:40: error: expected identifier before '__is_convertible' 1735 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 0, 1> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1735:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1736:40: error: expected identifier before '__is_convertible' 1736 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 1, 1> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1736:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1737:40: error: expected identifier before '__is_convertible' 1737 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 2, 1> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1737:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1738:40: error: expected identifier before '__is_convertible' 1738 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 3, 1> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1738:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1740:40: error: expected identifier before '__is_convertible' 1740 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 0, 2> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1740:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1741:40: error: expected identifier before '__is_convertible' 1741 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 1, 2> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1741:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1742:40: error: expected identifier before '__is_convertible' 1742 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 2, 2> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1742:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1743:40: error: expected identifier before '__is_convertible' 1743 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 3, 2> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1743:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1745:40: error: expected identifier before '__is_convertible' 1745 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 0, 3> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1745:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1746:40: error: expected identifier before '__is_convertible' 1746 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 1, 3> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1746:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1747:40: error: expected identifier before '__is_convertible' 1747 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 2, 3> : public false_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1747:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1748:40: error: expected identifier before '__is_convertible' 1748 | template <class _T1, class _T2> struct __is_convertible<_T1, _T2, 3, 3> : public true_type {}; | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1748:40: error: expected unqualified-id before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1751:14: error: expected class-name before '__is_convertible' 1751 | : public __is_convertible<_T1, _T2> | ^~~~~~~~~~~~~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/type_traits:1751:14: error: expected '{' before '__is_convertible' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility: In instantiation of 'static constexpr bool std::__1::pair<_T1, _T2>::_CheckArgs::__enable_explicit() [with _U1 = const long unsigned int&; _U2 = const long unsigned int&; _T1 = long unsigned int; _T2 = long unsigned int]': /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:414:87: required by substitution of 'template<bool _Dummy, typename std::__1::enable_if<typename std::__1::conditional<_MaybeEnable, std::__1::pair<long unsigned int, long unsigned int>::_CheckArgs, std::__1::__check_tuple_constructor_fail>::type::__enable_explicit<const long unsigned int&, const long unsigned int&>(), bool>::type <anonymous> > constexpr std::__1::pair<long unsigned int, long unsigned int>::pair(const long unsigned int&, const long unsigned int&) [with bool _Dummy = true; typename std::__1::enable_if<typename std::__1::conditional<_MaybeEnable, std::__1::pair<long unsigned int, long unsigned int>::_CheckArgs, std::__1::__check_tuple_constructor_fail>::type::__enable_explicit<const long unsigned int&, const long unsigned int&>(), bool>::type <anonymous> = <missing>]' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:1110:12: required from 'static std::__1::pair<_Size, _Size> std::__1::__murmur2_or_cityhash<_Size, 64>::__weak_hash_len_32_with_seeds(_Size, _Size, _Size, _Size, _Size, _Size) [with _Size = long unsigned int]' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:1118:41: required from 'static std::__1::pair<_Size, _Size> std::__1::__murmur2_or_cityhash<_Size, 64>::__weak_hash_len_32_with_seeds(const char*, _Size, _Size) [with _Size = long unsigned int]' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:1177:57: required from '_Size std::__1::__murmur2_or_cityhash<_Size, 64>::operator()(const void*, _Size) [with _Size = long unsigned int]' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:1256:47: required from 'size_t std::__1::__scalar_hash<_Tp, 2>::operator()(_Tp) const [with _Tp = std::__1::_PairT; size_t = long unsigned int]' /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:1314:20: required from here /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:354:53: error: incomplete type 'std::__1::is_convertible<const long unsigned int&, long unsigned int>' used in nested name specifier 354 | && (!is_convertible<_U1, first_type>::value | ^~~~~ /builddir/build/BUILD/sgxsdk-2.19.100.0/sdk/tlibcxx/include/utility:355:57: error: incomplete type 'std::__1::is_convertible<const long unsigned int&, long unsigned int>' used in nested name specifier 355 | || !is_convertible<_U2, second_type>::value);
__is_convertible is gcc builtin from g++13: https://github.com/gcc-mirror/gcc/commit/af85ad891703db220b25e7847f10d0bbec4becf4 Looks like libcxx "silently" fixed this via: https://github.com/llvm/llvm-project/commit/2040fde9097ae7753531c9c58332a933cbaaa43c#diff-3d2df1e1a88b47254c4ed1bc1fb735281c17bc8540d1c70c5c78d3bfd776b7ddR27 (note that __has_builtin argument is changed to "__is_convertible_to", from "is_convertible_to")
Thanks a lot for the information. We will check it.
BTW, rpmbuild introduces one compile option "-flto" which causes another build error as below. So my question is - Do we have to use this compile option? Thanks! g++ -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -Wnon-virtual-dtor -std=c++17 -fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG -ffunction-sections -fdata-sections -Wall -Wextra -Winit-self -Wpointer-arith -Wreturn-type -Waddress -Wsequence-point -Wformat-security -Wmissing-include-dirs -Wfloat-equal -Wundef -Wshadow -Wcast-align -Wconversion -Wredundant-decls -DITT_ARCH_IA64 -fcf-protection -Wnon-virtual-dtor -std=c++17 -fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG -ffunction-sections -fdata-sections -Wall -Wextra -Winit-self -Wpointer-arith -Wreturn-type -Waddress -Wsequence-point -Wformat-security -Wmissing-include-dirs -Wfloat-equal -Wundef -Wshadow -Wcast-align -Wconversion -Wredundant-decls -DITT_ARCH_IA64 -fcf-protection -Wnon-virtual-dtor -std=c++17 -fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG -ffunction-sections -fdata-sections -Wall -Wextra -Winit-self -Wpointer-arith -Wreturn-type -Waddress -Wsequence-point -Wformat-security -Wmissing-include-dirs -Wfloat-equal -Wundef -Wshadow -Wcast-align -Wconversion -Wredundant-decls -DITT_ARCH_IA64 -fcf-protection -Wnon-virtual-dtor -std=c++17 -fstack-protector-strong -O2 -D_FORTIFY_SOURCE=2 -UDEBUG -DNDEBUG -ffunction-sections -fdata-sections -Wall -Wextra -Winit-self -Wpointer-arith -Wreturn-type -Waddress -Wsequence-point -Wformat-security -Wmissing-include-dirs -Wfloat-equal -Wundef -Wshadow -Wcast-align -Wconversion -Wredundant-decls -DITT_ARCH_IA64 -fcf-protection -Werror -fpie -DOPENSSL_API_COMPAT=10101 manage_metadata.o enclave_creator_sign.o sign_tool.o parse_key_file.o util_st.o sgx_memset_s.o crypto_evp_digest.o tinyxml2.o loader.o se_detect.o se_trace.o se_map.o -L/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILD/sgxsdk-2.19.100.0/psw/urts/parser -pie -Wl,-z,relro,-z,now,-z,noexecstack -lpthread -lenclaveparser -lcrypto -o sgx_sign /sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILD/sgxsdk-2.19.100.0/common/inc/internal/thread_data.h:88:16: error: type 'struct _thread_data_t' violates the C++ One Definition Rule [-Werror=odr] 88 | typedef struct _thread_data_t | ^ /sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILD/sgxsdk-2.19.100.0/common/inc/internal/thread_data.h:88:16: note: a different type is defined in another translation unit 88 | typedef struct _thread_data_t | ^ /sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILD/sgxsdk-2.19.100.0/common/inc/internal/thread_data.h:90:17: note: the first difference of corresponding definitions is field 'self_addr' 90 | sys_word_t self_addr;has context menuComposeParagraph
Rather resorting to modifying Fedora specified compilation flags, firstly try to analyze the message and fix the cause. In this case, the message says: > type 'struct _thread_data_t' violates the C++ One Definition Rule So I guess this is really unwilling and I think it is better to get this fixed anyway. Note that unless the srpm is available we cannot post some further detailed advice. Or for now you can just remove "-Werror", which is the "real" cause which caused gcc warnings to error, Fedora packages usually don't use -Werror.
Got it. Thanks! We're working on some other compile issues caused by new build environment. Once it is done, a new version will be submitted.
The last build failure is as below. My question is - can we just skip to generate debuginfo package automatically since users don't need to debug libsgx_ptrace.so? Thanks! Error while writing index for `/disk2/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_ptrace.so': No debugging symbols gdb-add-index: No index was created for /home/se/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_ptrace.so gdb-add-index: [Was there no debuginfo? Was there already an index?] extracting debug info from /home/se/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_quote_ex_sim.so extracting debug info from /home/se/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_uae_service_sim.so extracting debug info from /home/se/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_urts_sim.so Missing separate debuginfo for /disk2/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_urts_sim.so Try: dnf --enablerepo='*debug*' install /usr/lib/debug/.build-id/bb/763be976f01300584049f5745f78fc64bb9194.debug Error while writing index for `/disk2/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_urts_sim.so': No debugging symbols gdb-add-index: No index was created for /home/se/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_urts_sim.so gdb-add-index: [Was there no debuginfo? Was there already an index?]
Again, unless you provide the srpm you are using, we cannot investigate the cause, we cannot post some further detailed advice. I guess this is because libsgx_ptrace.so is already stripped, which prevents debuginfo package from being generated. Or maybe Fedora build flags (which includes "-g" flag) are not correctly honored. So: * make it sure that libsgx_ptrace.so or so are not stripped during %build and %install * make it sure that Fedora compilation flags are correctly honored. But again, this is just a guess, I cannot provide further advice, because unless srpm is provided, we cannot investigate further. For the question "can we just skip to generate debuginfo package automatically", the answer is "no, unless it is really unavoidable".
By the way, > extracting debug info from /home/se/github/sgx-sdk/linux/installer/rpm/sdk/sgxsdk-2.19.100.0/BUILDROOT/sgxsdk-2.19.100.0-1.fc39.x86_64/usr/lib64/libsgx_quote_ex_sim.so Installing library with no soversion seems some mistake, usually libraries installed directly under %_libdir should have soversion (i.e. should be named as libfoo.so.X.Y.Z). Libraries without soversion should usually be installed under some package-specific directory (such as %_libdir/%name ).
Sorry for the delayed update. After the license change and various compiling errors against rawhide being fixed, new version of spec and srpm are ready for review now. SPEC: https://yunyings.fedorapeople.org/sgxsdk.spec SRPM: https://yunyings.fedorapeople.org/sgxsdk-2.19.100.0-1.fc39.src.rpm Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=99176101 Please help to review it again. Thanks!
The path for sharing the processed tarball(Source0 in spec) has changed, so I updated the spec again to use the right one: Source0: https://download.01.org/intel-sgx/sgx_repo/rpm_onespec/%{name}-%{version}.tar.gz The processed tarball is generated by extracting and running "./linux/installer/rpm/sdk/build.sh" upon the github release tarball from https://github.com/intel/linux-sgx/releases. Updates for spec & src.rpm are done. The fedorapeople links above remain valid. New Koji build has completed: https://koji.fedoraproject.org/koji/taskinfo?taskID=99214833 .
(In reply to Yunying Sun from comment #49) > Sorry for the delayed update. After the license change and various compiling > errors against rawhide being fixed, new version of spec and srpm are ready > for review now. > > SPEC: https://yunyings.fedorapeople.org/sgxsdk.spec > SRPM: https://yunyings.fedorapeople.org/sgxsdk-2.19.100.0-1.fc39.src.rpm > Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=99176101 > > Please help to review it again. Thanks! Some minor things. The "Requires" should go after the "BuildRequires". Consider using the %autosetup macro instead of the %setup one. Now the way you're creating the source strikes me a bit weird. So you get the github tarball, run a script and generate a different tarball that then is uploaded to some server? Isn't there a canonical release on github or somewhere else for that? I'll let other pitch in on that, however if you'd go that way the relevant script should be added in the source rpm alongside the SPEC and the process of creating the sources explained on a comment inside the SPEC. An example of modifying the sources through instructions from the SPEC: https://src.fedoraproject.org/rpms/python-setuptools/pull-request/96#_1__6 And another example of having a script in the package tree to create modified sources (vendor_rust.py in this case): https://src.fedoraproject.org/rpms/python-cryptography/tree/rawhide
(In reply to Charalampos Stratakis from comment #51) > (In reply to Yunying Sun from comment #49) > > Sorry for the delayed update. After the license change and various compiling > > errors against rawhide being fixed, new version of spec and srpm are ready > > for review now. > > > > SPEC: https://yunyings.fedorapeople.org/sgxsdk.spec > > SRPM: https://yunyings.fedorapeople.org/sgxsdk-2.19.100.0-1.fc39.src.rpm > > Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=99176101 > > > > Please help to review it again. Thanks! > > Some minor things. > > The "Requires" should go after the "BuildRequires". Agree to change it. > Consider using the %autosetup macro instead of the %setup one. $ cat /etc/os-release NAME="Fedora Linux" VERSION="39 (Rawhide Prerelease)" ID=fedora VERSION_ID=39 $ rpmbuild --eval=%autosetup error: lua script failed: attempt to index a nil value I did an experiment on fedora 39 and found %autosetup macro value is nil. So could you please point how to use it? > Now the way you're creating the source strikes me a bit weird. So you get > the github tarball, run a script and generate a different tarball that then > is uploaded to some server? Isn't there a canonical release on github or > somewhere else for that? I'll let other pitch in on that, however if you'd > go that way the relevant script should be added in the source rpm alongside > the SPEC and the process of creating the sources explained on a comment > inside the SPEC. > > An example of modifying the sources through instructions from the SPEC: > https://src.fedoraproject.org/rpms/python-setuptools/pull-request/96#_1__6 > > And another example of having a script in the package tree to create > modified sources (vendor_rust.py in this case): > https://src.fedoraproject.org/rpms/python-cryptography/tree/rawhide If my understanding is right, we discussed this question before. There are some pre-build binaries like aes which need to be downloaded first.
(In reply to xiangquan.liu from comment #52) > (In reply to Charalampos Stratakis from comment #51) > > (In reply to Yunying Sun from comment #49) > > > Sorry for the delayed update. After the license change and various compiling > > > errors against rawhide being fixed, new version of spec and srpm are ready > > > for review now. > > > > > > SPEC: https://yunyings.fedorapeople.org/sgxsdk.spec > > > SRPM: https://yunyings.fedorapeople.org/sgxsdk-2.19.100.0-1.fc39.src.rpm > > > Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=99176101 > > > > > > Please help to review it again. Thanks! > > > > Some minor things. > > > > The "Requires" should go after the "BuildRequires". > Agree to change it. > > > Consider using the %autosetup macro instead of the %setup one. > > $ cat /etc/os-release > NAME="Fedora Linux" > VERSION="39 (Rawhide Prerelease)" > ID=fedora > VERSION_ID=39 > > $ rpmbuild --eval=%autosetup > error: lua script failed: attempt to index a nil value > > I did an experiment on fedora 39 and found %autosetup macro value is nil. So > could you please point how to use it? > %autosetup is a drop-in replacement for %setup, adding some more convenience for possible future patches: https://rpm-software-management.github.io/rpm/manual/autosetup.html > > Now the way you're creating the source strikes me a bit weird. So you get > > the github tarball, run a script and generate a different tarball that then > > is uploaded to some server? Isn't there a canonical release on github or > > somewhere else for that? I'll let other pitch in on that, however if you'd > > go that way the relevant script should be added in the source rpm alongside > > the SPEC and the process of creating the sources explained on a comment > > inside the SPEC. > > > > An example of modifying the sources through instructions from the SPEC: > > https://src.fedoraproject.org/rpms/python-setuptools/pull-request/96#_1__6 > > > > And another example of having a script in the package tree to create > > modified sources (vendor_rust.py in this case): > > https://src.fedoraproject.org/rpms/python-cryptography/tree/rawhide > > If my understanding is right, we discussed this question before. There are > some pre-build binaries like aes which need to be downloaded first. Could you point me to those discussions?
(In reply to Charalampos Stratakis from comment #53) > (In reply to xiangquan.liu from comment #52) > > (In reply to Charalampos Stratakis from comment #51) > > > (In reply to Yunying Sun from comment #49) > > > > Sorry for the delayed update. After the license change and various compiling > > > > errors against rawhide being fixed, new version of spec and srpm are ready > > > > for review now. > > > > > > > > SPEC: https://yunyings.fedorapeople.org/sgxsdk.spec > > > > SRPM: https://yunyings.fedorapeople.org/sgxsdk-2.19.100.0-1.fc39.src.rpm > > > > Koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=99176101 > > > > > > > > Please help to review it again. Thanks! > > > > > > Some minor things. > > > > > > The "Requires" should go after the "BuildRequires". > > Agree to change it. > > > > > Consider using the %autosetup macro instead of the %setup one. > > > > $ cat /etc/os-release > > NAME="Fedora Linux" > > VERSION="39 (Rawhide Prerelease)" > > ID=fedora > > VERSION_ID=39 > > > > $ rpmbuild --eval=%autosetup > > error: lua script failed: attempt to index a nil value > > > > I did an experiment on fedora 39 and found %autosetup macro value is nil. So > > could you please point how to use it? > > > > %autosetup is a drop-in replacement for %setup, adding some more convenience > for possible future patches: > https://rpm-software-management.github.io/rpm/manual/autosetup.html > > > > Now the way you're creating the source strikes me a bit weird. So you get > > > the github tarball, run a script and generate a different tarball that then > > > is uploaded to some server? Isn't there a canonical release on github or > > > somewhere else for that? I'll let other pitch in on that, however if you'd > > > go that way the relevant script should be added in the source rpm alongside > > > the SPEC and the process of creating the sources explained on a comment > > > inside the SPEC. > > > > > > An example of modifying the sources through instructions from the SPEC: > > > https://src.fedoraproject.org/rpms/python-setuptools/pull-request/96#_1__6 > > > > > > And another example of having a script in the package tree to create > > > modified sources (vendor_rust.py in this case): > > > https://src.fedoraproject.org/rpms/python-cryptography/tree/rawhide > > > > If my understanding is right, we discussed this question before. There are > > some pre-build binaries like aes which need to be downloaded first. > > Could you point me to those discussions? This is sub-ticket of https://bugzilla.redhat.com/show_bug.cgi?id=2030595 in which you can find those discussions.
The spec file says: > # The entire source code is BSD, except some Intel signed binaries > # include libsgx_{qve,tdqe,id_enclave,pce,qe3,le,qe,pve}.signed.so are > # Intel "redistributable without modification" binary license, and > # some third party projects are under other licenses 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 I don't believe the comment "# Intel "redistributable without modification" binary license" is correct and in sync with upstream (packaged) License.txt file that says: libsgx_le.signed.so, libsgx_pce.signed.so, libsgx_pve.signed.so, libsgx_qe.signed.so, libsgx_pse_pr.signed.so, libsgx_pse_pr_2.signed.so and libsgx_pse_op.signed.so are licensed under 3-Clause BSD License. 3-Clause BSD license snippet: Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met: [...]
FWIW, although it's probably a small detail: the sample codes packaged in the subpackage sgxsdk-samples cannot be built: sgx_edger8r installed by this package cannot be located, as the sample code Makefiles try to locate it in one of the following (depending on setup): ${SGX_SDK:-/opt/intel/sgxsdk}/bin/{x86,x64}/. I think those are the original paths before it was moved out of /opt.
> %package -n sgxsdk-samples > Summary: Intel SGX SDK Sample Code > Requires: %{name} = %{version}-%{release} > Requires: libsgx-urts >= %{version}-%{release} > Requires: libsgx-enclave-common >= %{version}-%{release} Another nit: should probably add other requires, e.g., Requires: gcc-c++. Perhaps sgxsdk-samples subpackage can be added later if you don't want to address these now -- after sgx-aesm-service (that provides both libsgx-enclave-common and libsgx-urt [and require many of the above discussed fixes, too]) is in Fedora; until then its dependencies will not be satisfiable anyway.
(In reply to Cestmir Kalina from comment #58) > FWIW, although it's probably a small detail: the sample codes packaged in > the subpackage sgxsdk-samples cannot be built: sgx_edger8r installed by this > package cannot be located, as the sample code Makefiles try to locate it in > one of the following (depending on setup): > ${SGX_SDK:-/opt/intel/sgxsdk}/bin/{x86,x64}/. I think those are the original > paths before it was moved out of /opt. Could you please review SampleEnclave which I modified its Makefile already? I think SampleEnclave can be built successfully.
(In reply to Cestmir Kalina from comment #59) > > %package -n sgxsdk-samples > > Summary: Intel SGX SDK Sample Code > > Requires: %{name} = %{version}-%{release} > > Requires: libsgx-urts >= %{version}-%{release} > > Requires: libsgx-enclave-common >= %{version}-%{release} > > Another nit: should probably add other requires, e.g., Requires: gcc-c++. > > Perhaps sgxsdk-samples subpackage can be added later if you don't want to > address these now -- after sgx-aesm-service (that provides both > libsgx-enclave-common and libsgx-urt [and require many of the above > discussed fixes, too]) is in Fedora; until then its dependencies will not be > satisfiable anyway. I agree. Thanks.
(In reply to xiangquan.liu from comment #60) > (In reply to Cestmir Kalina from comment #58) > > FWIW, although it's probably a small detail: the sample codes packaged in > > the subpackage sgxsdk-samples cannot be built: sgx_edger8r installed by this > > package cannot be located, as the sample code Makefiles try to locate it in > > one of the following (depending on setup): > > ${SGX_SDK:-/opt/intel/sgxsdk}/bin/{x86,x64}/. I think those are the original > > paths before it was moved out of /opt. > > Could you please review SampleEnclave which I modified its Makefile already? > I think SampleEnclave can be built successfully. I pulled the latest SRPM and I am afraid that the issue persists, even in SampleEnclave, on a fresh Fedora rawhide install (with sgxsdk-samples dependencies manually compiled and installed so that $(SGX_SDK) default is not present). # relevant snippets of: SampleCode/SampleEnclave/Makefile 34 SGX_SDK ?= /opt/intel/sgxsdk .. 51 ifeq ($(SGX_ARCH), x86) .. 55 SGX_EDGER8R := $(SGX_SDK)/bin/x86/sgx_edger8r 56 else .. 60 SGX_EDGER8R := $(SGX_SDK)/bin/x64/sgx_edger8r 61 endif .. 63 # For onespec, tools like sgx_sign are moved to /usr/bin .. 68 ifeq ($(wildcard $(SGX_EDGER8R)),) 69 SGX_EDGER8R := sgx_edger8r 70 endif .. 233 App/Enclave_u.h: $(SGX_EDGER8R) Enclave/Enclave.edl $ make # hw mode, debug build, no mitigation, per instructions in README.txt make[1]: Entering directory '/buildtmpfs/SampleCode/SampleEnclave' make[1]: *** No rule to make target 'sgx_edger8r', needed by 'App/Enclave_u.h'. Stop. Changing line 69 to something like SGX_EDGER8R := $(shell command -v sgx_edger8r) instead does allow the compilation to proceed.
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 `-samples` package naming is not common in Fedora, there's many more instances of `-examples`. And typically example code goes into /usr/share/doc with `%doc` There's also a compiled .so file sneaking in there: sgxsdk-samples.x86_64: E: arch-dependent-file-in-usr-share /usr/share/sgxsdk/SampleCode/RemoteAttestation/sample_libcrypto/libsample_libcrypto.so which seems strange since this is essentially documentation. Maybe `%exclude` it in %files or delete it after %make_install 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 Here's some small changes: typo fix, trailing whitespace, use %autosetup: --- srpm/sgxsdk.spec 2023-07-01 12:28:20.001294623 -0400 +++ srpm-unpacked/sgxsdk.spec 2023-07-01 14:26:31.008591969 -0400 @@ -1,4 +1,4 @@ -# SGX SDK is for development use only hence no deubg package needed. +# SGX SDK is for development use only hence no debug package needed. %define debug_package %{nil} Name: sgxsdk @@ -43,8 +43,8 @@ BuildRequires: nasm ExclusiveArch: x86_64 %description -The Intel SGX SDK is a collection of APIs, libraries, documentation, and tools -that allows software developers to create and debug Intel SGX enabled +The Intel SGX SDK is a collection of APIs, libraries, documentation, and tools +that allows software developers to create and debug Intel SGX enabled applications in C/C++. %package -n sgxsdk-samples @@ -54,12 +54,12 @@ Requires: libsgx-urts >= %{version Requires: libsgx-enclave-common >= %{version}-%{release} %description -n sgxsdk-samples -The Intel SGX SDK sample code projects show developers how to create an +The Intel SGX SDK sample code projects show developers how to create an enclave, how to use C++11 library inside the enclave, how to do local attestation and remote attestation, etc. %prep -%setup -qc +%autosetup -c %build %make_build
Sorry for the delay. Now updated spec file and srpm are available at: https://yunyings.fedorapeople.org/sgxsdk.spec https://yunyings.fedorapeople.org/sgxsdk-2.20.100.0-1.fc39.src.rpm To address recent comments since Jun 28, the spec changes included in this update: 1. Removed license statement about Intel binaries in comments. 2. Added comment to clarify how the repacked tarball on 01.org is generated from github source. 3. Added "Requires gcc-c++", and moved "Requires:" behind the "Buildrequires:" section. 4. Using "%autosetup" instead of "%setup". 5. Removed subpackage "sgxsdk-samples" stuff, which will be added later after sgx-aesm-service available Fedora. 6. Updated to release 2.20.100(which is upstream 2.20 release plus some unpublished code changes to fix compiling errors found on Fedora rawhide). Please help to review it again. Thanks.
> 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/
> > 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 Furthermore, code under the CC0 license is now broadly forbidden in new packages addd to Fedora: https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/RRYM3CLYJYW64VSQIXY6IF3TCDZGS6LM/ AFAICT, this CC0 applies to sdk/tlibc/stdlib/malloc.c so that is a problem that needs resolving. Previous versions of DL malloc were under a public domain dedication rather than CC0 the SDK currently bundles version 2.8.6, but the older version 2.8.4: https://gee.cs.oswego.edu/pub/misc/malloc-2.8.4.c was under the https://spdx.org/licenses/CC-PDDC.html which is permissible for Fedora.
# 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 I think this license list is probably incomplete. Running the licensecheck -r . | sed -e 's/^.*: //' -e 's/\*No copyright\* //' | sort | uniq -c 2653 Apache License 2.0 9 Apache License 2.0 [generated file] 1 BSD 2-clause FreeBSD License 90 BSD 2-Clause License 1 BSD 2-Clause License Apache License 2.0 5 BSD 2-clause NetBSD License BSD 2-Clause License 4355 BSD 3-Clause License 3 BSD 3-Clause License Apache License 2.0 1 BSD 3-Clause License BSD 2-Clause License 1 BSD 3-Clause License BSD 2-Clause License Eclipse Public License 1.0 22 BSD 3-Clause License GNU General Public License, Version 2 1 BSD 4-Clause License 1 BSD 4-Clause License BSD 3-Clause License 2 BSD-4-Clause (University of California-Specific) 4 FSF All Permissive License 2 FSF Unlimited License [generated file] 1 GNU General Public License v2.0 or later Apache License (v2.0) or GNU General Public License (v2.0 or later) 2 GNU General Public License, Version 2 102 ISC License 1 Microsoft Public License BSD 3-Clause License 1464 MIT License 2 MIT License BSD 3-Clause License 129 OpenSSL License 4 OpenSSL License Apache License 1.0 10 OpenSSL License BSD 3-Clause License 5 Public domain 3 Public domain BSD 3-Clause License 9 SSLeay 18 Standard ML of New Jersey License 6 University of Illinois/NCSA Open Source License 4616 UNKNOWN 56 UNKNOWN [generated file] 2 zlib License Now I wouldn't blindly trust the output of this tool, because it certainly makes mistakes, and not all source is relevant to what goes into the binary RPM. For each reported license scenario here, need to check if there is at least 1 file that's relevant to the binary RPM. At least the ISC license appears relevant and isn't listed.
(In reply to Daniel Berrangé from comment #65) > 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. Sorry, I missed these: renaming package to sgxsdk-devel, fixing typo fixes and trailing whitespaces. Will fix both soon. About the bundle thing Cole suggested in comment 63, we don't quite understand. Are you suggesting to add all third party tools that linux-sgx is using their old versions instead of the up-to-date system-provided versions, to spec file with "Provides: bundled(xxx) = <the old versions>"? Package guidance says: "All packages whose upstreams have no mechanism to build against system libraries MAY opt to carry bundled libraries" does that mean bundled libraries are not necessarily listed in spec file? > > > > 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. We removed sub-package sgxsdk-samples because as Cestmir stated in comment 59: "Perhaps sgxsdk-samples subpackage can be added later if you don't want to address these now -- after sgx-aesm-service (that provides both libsgx-enclave-common and libsgx-urt [and require many of the above discussed fixes, too]) is in Fedora; until then its dependencies will not be satisfiable anyway." Since sgx-aesm-service depends on SGX SDK package reversely, could you confirm if reviewing sgx-sdk and sgx-aesm-service in pair could satisfy the cross-dependency for compiling? If yes, I can add the sgxsdk-samples back(and rename it to sgxsdk-examples as Cole suggested in comment 63). > > > # 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 We will update the license fields to follow the new expressions and standard. > > > # 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 There are some unpublished fixes(needed for compiling source on Fedora) not available on public github linux-sgx repo yet. The fixes are available only in the tarball contained in SRPM @ https://yunyings.fedorapeople.org/sgxsdk-2.20.100.0-1.fc39.src.rpm. > > > %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. We will fix this. > > > %{_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/ We will expand the list of %{_includedir}/sgxsdk/ as you suggested above. Do we need to expand %{_datadir}/sgxsdk/ and %{_libdir}/sgx-gdb-plugin/ to its next level too? > Furthermore, code under the CC0 license is now broadly forbidden in new packages addd to Fedora > https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/RRYM3CLYJYW64VSQIXY6IF3TCDZGS6LM/ Latest post on this thread says: "Realistically, we may also want to continue to allow CC0 covered code for a short period of time for new packages that are just getting into Fedora until some percolating has happened in those upstream communities". Can we ship this package still with CC0 license, meanwhile we notify upstream about the need for a license change? We can refresh package license after upstream license changes. Is this acceptable? > At least the ISC license appears relevant and isn't listed. It seems the ISC license only appears in the "OpenBSD Copyright Policy" part in License.txt. Not related to any source code?
(In reply to Yunying Sun from comment #68) > About the bundle thing Cole suggested in comment 63, we don't quite > understand. > Are you suggesting to add all third party tools that linux-sgx is using > their old versions instead of the up-to-date system-provided versions, to > spec file with "Provides: bundled(xxx) = <the old versions>"? > > Package guidance says: > "All packages whose upstreams have no mechanism to build against system > libraries MAY opt to carry bundled libraries" > does that mean bundled libraries are not necessarily listed in spec file? Normally you have to link against the system libraries. This guideline allows you to skip that and instead rely on bundled libraries. When doing that though, you need to add Provides: bundled(xxx) = <the version you bundle>" for each library you have bundled. > > > 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. > We removed sub-package sgxsdk-samples because as Cestmir stated in comment > 59: > "Perhaps sgxsdk-samples subpackage can be added later if you don't want to > address these now -- after sgx-aesm-service (that provides both > libsgx-enclave-common and libsgx-urt [and require many of the above > discussed fixes, too]) is in Fedora; until then its dependencies will not be > satisfiable anyway." > > Since sgx-aesm-service depends on SGX SDK package reversely, could you > confirm if reviewing sgx-sdk and sgx-aesm-service in pair could satisfy the > cross-dependency for compiling? Having a circular dependency at package build time is a problem, because packages are built individually. IIUC, that's not the case with the samples though. They are just source code, and user can build themselves against the sgex-aesm-service package(s). > If yes, I can add the sgxsdk-samples back(and rename it to sgxsdk-examples > as Cole suggested in comment 63). I'd prefer to see it kept, depsite what Cestmir said. > > > # 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 > There are some unpublished fixes(needed for compiling source on Fedora) not > available on public github linux-sgx repo yet. > The fixes are available only in the tarball contained in SRPM @ > https://yunyings.fedorapeople.org/sgxsdk-2.20.100.0-1.fc39.src.rpm. > > > > %{_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/ > We will expand the list of %{_includedir}/sgxsdk/ as you suggested above. > Do we need to expand %{_datadir}/sgxsdk/ and %{_libdir}/sgx-gdb-plugin/ to > its next level too? I don't think it is needed to list every .py file in the sgx-gdb-plugin dir, just the dir ittself. For %{_datadir}/sgxsdk/ there are only two files, so might as well just list them directly. > > Furthermore, code under the CC0 license is now broadly forbidden in new packages addd to Fedora > > https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/RRYM3CLYJYW64VSQIXY6IF3TCDZGS6LM/ > Latest post on this thread says: > "Realistically, we may also want to continue to allow CC0 covered code for a > short period of time for new packages that are just getting into Fedora > until some percolating has happened in those upstream communities". > > Can we ship this package still with CC0 license, meanwhile we notify > upstream about the need for a license change? > We can refresh package license after upstream license changes. Is this > acceptable? I looked for other examples of package reviews since this CC0 license change, and the recent additino os wasi-libc had to replace the dlmalloc impl with a different one under MIT license. They could have chosen to use an older versin of dlmalloc under the previous license, don't know why they chose a completely new malloc impl. So I think the precedent is that after 1 year since the announcement, adding new packages with CC0 content is no longer permitted. > > At least the ISC license appears relevant and isn't listed. > It seems the ISC license only appears in the "OpenBSD Copyright Policy" part > in License.txt. Not related to any source code? I wouldn't trust the License.txt file summary to be accurate. What matters is the individual files' declared licenses, and I see lots of source files with ISC license reported by the license scanner, and its identification appears to be correct in these cases: ./sdk/pthread/pthread_cond.cpp: ISC License ./sdk/pthread/pthread_mutex.cpp: ISC License ./sdk/pthread/pthread_once.cpp: ISC License ./sdk/pthread/pthread_rwlock.cpp: ISC License ./sdk/pthread/pthread_tls.cpp: ISC License ./common/inc/tlibc/complex.h: ISC License ./common/inc/tlibc/inttypes.h: ISC License ./common/inc/tlibc/stdint.h: ISC License ./sdk/tlibc/gen/fpclassify.c: ISC License ./sdk/tlibc/gen/fpclassifyl.c: ISC License ./sdk/tlibc/gen/isfinite.c: ISC License ./sdk/tlibc/gen/isfinitel.c: ISC License ...snip... ./sdk/tlibc/math/e_expl.c: ISC License ./sdk/tlibc/math/e_log10l.c: ISC License ./sdk/tlibc/math/e_log2l.c: ISC License ./sdk/tlibc/math/e_logl.c: ISC License ...snip...
(In reply to Daniel Berrangé from comment #69) > (In reply to Yunying Sun from comment #68) > > If yes, I can add the sgxsdk-samples back(and rename it to sgxsdk-examples > > as Cole suggested in comment 63). > > I'd prefer to see it kept, depsite what Cestmir said. FWIW I agree that this makes more sense if the two packages are to be reviewed and included concurrently, so please ignore the relevant sections of comment 59.
Thanks for clarifying. I will add sub-package sgxsdk-samples back to spec file. Meanwhile should we rename it to sgxsdk-examples as Cole suggested in comment 63? We will update the License field to comply with SPDX declarations, and sync internally with upstream about the possibility to use the older version of malloc to avoid the CC0 license issue. We will also update the file lists in %files field. It may take some time. We will come back with updated spec and srpm asap.
Using '-examples' appears to be the more common convention in Fedora - 210 for '-examples' and only 13 for '-samples', so go with '-examples'
(In reply to Daniel Berrangé from comment #69) > I looked for other examples of package reviews since this CC0 license > change, and > the recent additino os wasi-libc had to replace the dlmalloc impl with a > different > one under MIT license. They could have chosen to use an older versin of > dlmalloc > under the previous license, don't know why they chose a completely new malloc > impl. So I think the precedent is that after 1 year since the announcement, > adding new packages with CC0 content is no longer permitted. Hi Daniel, About the CC0 license issue, linux-sgx team tended not to downgrade dlmalloc to the CC-PDDC licensed v2.8.4 for security concerns. That leaves two options as we can see: Option 1: We relicense sdk/tlibc/stdlib/malloc.c to BSD by adding a secondary header like what openjdk does: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/concurrent/AbstractExecutorService.java This is the way the author of dlmalloc recommended. Will this work for Fedora? Option 2: We add note to explain that the original dlmalloc is under CC0, and linux-sgx changes to it is under BSD. Like what wasi-libc has in its License: https://github.com/WebAssembly/wasi-libc/blob/main/LICENSE Despite wasi-libc seems still using dlmalloc as its default: https://github.com/WebAssembly/wasi-libc/blob/main/Makefile#L19 In its package spec, CC0 does not need to be listed anymore in License field: https://src.fedoraproject.org/rpms/wasi-libc/blob/rawhide/f/wasi-libc.spec#_9. Which option do you suggest we take? Or any other alternatives?
(In reply to Yunying Sun from comment #73) > (In reply to Daniel Berrangé from comment #69) > > I looked for other examples of package reviews since this CC0 license > > change, and > > the recent additino os wasi-libc had to replace the dlmalloc impl with a > > different > > one under MIT license. They could have chosen to use an older versin of > > dlmalloc > > under the previous license, don't know why they chose a completely new malloc > > impl. So I think the precedent is that after 1 year since the announcement, > > adding new packages with CC0 content is no longer permitted. > Hi Daniel, > > About the CC0 license issue, linux-sgx team tended not to downgrade dlmalloc > to the CC-PDDC licensed v2.8.4 for security concerns. > That leaves two options as we can see: > > Option 1: We relicense sdk/tlibc/stdlib/malloc.c to BSD by adding a > secondary header like what openjdk does: > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/ > util/concurrent/AbstractExecutorService.java > This is the way the author of dlmalloc recommended. Will this work for > Fedora? > > Option 2: We add note to explain that the original dlmalloc is under CC0, > and linux-sgx changes to it is under BSD. Like what wasi-libc has in its > License: > https://github.com/WebAssembly/wasi-libc/blob/main/LICENSE > Despite wasi-libc seems still using dlmalloc as its default: > https://github.com/WebAssembly/wasi-libc/blob/main/Makefile#L19 > In its package spec, CC0 does not need to be listed anymore in License > field: > https://src.fedoraproject.org/rpms/wasi-libc/blob/rawhide/f/wasi-libc. > spec#_9. > > Which option do you suggest we take? Or any other alternatives? I don't have sufficient confidence in either of these options of changing the license myself, so I've requested Fedora Legal to give an opinion: https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/DOE4BT2OCTAFQYECG3ZCQMKOIINOYZUX/
We've just made an update for spec and srpm, also updated from release 2.20 to 2.22: https://yunyings.fedorapeople.org/sgxsdk-devel.spec https://yunyings.fedorapeople.org/sgxsdk-devel-2.22.100.0-1.fc40.src.rpm Koji build succeeded: https://koji.fedoraproject.org/koji/taskinfo?taskID=106465995 Main updates: 1. Main binary package is renamed to sgxsdk-devel, sample code package is added back and renamed to sgxsdk-examples. 2. Fixed typo fixes/trailing whitespace in spec file. 3. Added dependent and provided third party libraries as bundled. 4. Expanded top level paths to list files and folders being packaged. 5. Updated licenses to follow SPDX expressions. Open issue: 1. The dlmalloc CC0 license issue is not included this time in "License:" field. In email author of dlmalloc is willing to change license, but new release with new license is not published yet. We will update SGX release, as well as this package, to incorporate new dlmalloc version once its license update is done.
(In reply to Yunying Sun from comment #75) > We've just made an update for spec and srpm, also updated from release 2.20 > to 2.22: > https://yunyings.fedorapeople.org/sgxsdk-devel.spec > https://yunyings.fedorapeople.org/sgxsdk-devel-2.22.100.0-1.fc40.src.rpm > > Koji build succeeded: > https://koji.fedoraproject.org/koji/taskinfo?taskID=106465995 I don't know what has changed, but I am unable to reproduce that myself. It fails to build in both Fedora 38 and rawhide, both in koji and locally https://koji.fedoraproject.org/koji/taskinfo?taskID=106842730 https://koji.fedoraproject.org/koji/taskinfo?taskID=106841867 > Main updates: > 1. Main binary package is renamed to sgxsdk-devel, sample code package is > added back and renamed to sgxsdk-examples. So slight mis-understanding here. The package / specfile should still be called 'sgxsdk', but it should create a sgxsgk-devel binary RPM as output. This is achieved by leaving the defualt %files section empty, meaning it won't create a 'sgxsdk' binary RPM, only the various sub-RPMs we define. This is the change that ought to achieve that, though I can't test it fully since I can't get the package to successfully build: $ diff -u sgxsdk-devel.spec~ sgxsdk-devel.spec --- sgxsdk-devel.spec~ 2023-09-21 04:18:48.000000000 +0100 +++ sgxsdk-devel.spec 2023-09-28 14:00:27.984540597 +0100 @@ -1,7 +1,7 @@ # SGX SDK is for development use only hence no deubg package needed. %define debug_package %{nil} -Name: sgxsdk-devel +Name: sgxsdk Version: 2.22.100.0 Release: 1%{?dist} Summary: Intel SGX SDK @@ -15,7 +15,7 @@ # ./linux/installer/rpm/sdk/build.sh to update spec and repack tarball. # Since no network access is possible for Fedora package build system, # the pre-downloaded and repacked tarball is shared on 01.org. -Source0: https://download.01.org/intel-sgx/sgx_repo/rpm_onespec/%{name}-%{version}.tar.gz +Source0: https://download.01.org/intel-sgx/sgx_repo/rpm_onespec/%{name}-devel-%{version}.tar.gz BuildRequires: autoconf BuildRequires: automake @@ -67,13 +67,22 @@ that allows software developers to create and debug Intel SGX enabled applications in C/C++. -%package -n sgxsdk-examples +%package devel +Summary: Intel SGX SDK + +%description devel +The Intel SGX SDK is a collection of APIs, libraries, documentation, and tools +that allows software developers to create and debug Intel SGX enabled +applications in C/C++. + + +%package examples Summary: Intel SGX SDK Sample Code Requires: %{name} = %{version}-%{release} Requires: libsgx-urts >= %{version}-%{release} Requires: libsgx-enclave-common >= %{version}-%{release} -%description -n sgxsdk-examples +%description examples The Intel SGX SDK sample code projects show developers how to create an enclave, how to use C++11 library inside the enclave, how to do local attestation and remote attestation, etc. @@ -87,7 +96,7 @@ %install %make_install -%files +%files devel %license License.txt %{_bindir}/sgx_config_cpusvn %{_bindir}/sgx_edger8r @@ -151,7 +160,7 @@ %{_datadir}/sgxsdk/libc++_Changes_SGX.txt %exclude %{_datadir}/sgxsdk/SampleCode/ -%files -n sgxsdk-examples +%files examples %{_datadir}/sgxsdk/SampleCode/ %changelog > 5. Updated licenses to follow SPDX expressions. This doesn't pass a check with 'license-validate' license-validate "BSD-3-Clause AND Apache-2.0 AND MIT AND OpenSSL AND ISC AND BSD-2-Clause AND GPL-2.0-only AND SMLNJ AND NCSA AND BSD-2-Clause-NetBSD AND Apache-1.0 AND FSFAP AND BSD-4-Clause-UC AND FSFUL AND Zlib AND (Apache-2.0 OR GPL-2.0-or-later) AND EPL-1.0 AND MS-PL AND BSD-4-Clause" No terminal defined for '-' at line 1 col 126 only AND SMLNJ AND NCSA AND BSD-2-Clause-NetBSD AND Apache-1.0 AND FSFAP AND BSD ^ Expecting: {'AND', 'OR'} the problem is that "BSD-2-Clause-NetBSD" is not an approved Fedora license https://docs.fedoraproject.org/en-US/legal/allowed-licenses/ Fortunately this time it seems this is just a minor technicality: https://gitlab.com/fedora/legal/fedora-license-data/-/issues/194 It was deprecated by SPDX, because it was legally identical to BSD-2-Clause. IOW, just remove the mention of "BSD-2-Clause-NetBSD" > Open issue: > 1. The dlmalloc CC0 license issue is not included this time in "License:" > field. In email author of dlmalloc is willing to change license, but new > release with new license is not published yet. > We will update SGX release, as well as this package, to incorporate new > dlmalloc version once its license update is done. Yep, understood.
Thanks again Daniel for the review. I've fixed the package name and license issue you mentioned. Currently we are seeing a different build error both local and on koji. We are working on fixes. I will update spec and srpm asap once compiling issue is fixed.
We have fixed package build errors, removed license of "BSD-2-Clause-NetBSD", and renamed spec file back to sgxsdk.spec while main package remaining as sgxsdk-devel.rpm. Updated spec and srpm: https://yunyings.fedorapeople.org/sgxsdk.spec https://yunyings.fedorapeople.org/sgxsdk-2.22.100.0-1.fc40.src.rpm Package built successfully both locally and on koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=107625704 The re-packed source tarball which contains prebuilt binaries and dcap release is shared on 01.org: https://download.01.org/intel-sgx/sgx_repo/rpm_onespec/ The re-packed sgxsdk tarball can also be made by unzipping the sgxsdk-2.22.100.0.tar.gz in srpm, running "make preparation", then "./linux/installer/rpm/sdk/build.sh".
FYI, I'm finding that the RPM still fails to buld for me about 50% of the time. This strongly suggests a makefile dependency is missing, causing non-deterministic behaviour with parallel builds When it fails I see this as the first sign of errors: CMake Error: CMAKE_C_COMPILER not set, after EnableLanguage CMake Error at /usr/share/cmake/Modules/CheckFunctionExists.cmake:86 (try_compile): Failed to configure test project build system. Call Stack (most recent call first): src/CMakeLists.txt:16 (check_function_exists) -- Configuring incomplete, errors occurred! make[4]: *** [Makefile:106: cbor_untrusted] Error 1 make[3]: *** [Makefile:96: cbor_untrusted] Error 2 make[2]: *** [Makefile.source:265: utls] Error 2 make[2]: *** Waiting for unfinished jobs.... Will attach the full logs
> # 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. > # Since no network access is possible for Fedora package build system, > # the pre-downloaded and repacked tarball is shared on 01.org. > Source0: https://download.01.org/intel-sgx/sgx_repo/rpm_onespec/%{name}-%{version}.tar.gz I've tried to follow these instructions again, assuming it was fixed since I previously raised the problem in August (https://bugzilla.redhat.com/show_bug.cgi?id=2085444#c65) I had to 'make sdk' after 'make preparation' and before running 'build.sh', otherwise files it was looking for don't exist. At the end of 'build.sh' there is a sgxsdk-2.22.100.3.tar.gz created, but its content don't resemble the contents of sgxsdk-2.22.100.0.tar.gz that is in the src.rpm provided in this review. The build.sh appears to create a tarball that contains exclusively a pre-built set of binaries, while the src.rpm here contains actual source, along with a few compiled binaries acquired from other tarballs that 'download_prebuilt.sh' acquired. Looking at the files in the sgxsdk-2.22.100.0.tar.gz many of them do not correspond to files that I can find in https://github.com/intel/linux-sgx, nor do they correspond to files in the linux-sgx-sgx_2.22.tar.gz provided at https://github.com/intel/linux-sgx/releases/tag/sgx_2.22 The 'make preparation' step downloads a file https://download.01.org/intel-sgx/sgx-linux/2.22/optimized_libs_2.22.tar.gz whose contents appear to be copied into sgxsdk-2.22.100.0.tar.gz. This optimized_libs_2.22.tar.gz contains a Master_EULA_for_Intel_Sw_Development_Products.pdf file that imposes restrictive licensing on anyone receiving this tarball of pre-build libraries. I don't think that will be acceptable for Fedora. Given that I cannot validate the provenance of the sgxsdk-2.22.100.3.tar.gz tarball in the provided src.rpm I'm not willing to approve this package. IMHO the whole idea of pre-generating a custom sgxsdk-2.22.100.3.tar.gz for the RPM is flawed and needs to be re-thought. This RPM needs to be built using the official pristine released linux-sgx-sgx_2.22.tar.gz from https://github.com/intel/linux-sgx/releases/tag/sgx_2.22 Separately the src.rpm should also contain the additional tarballs (that download_prebuilt.sh would otherwise fetch) with the requisite signed pre-compiled binaries, so we have clear separation of what parts are from the pristine source release and what parts are pre-compiled. The questionable EULA in optimized_libs_2.22.tar.gz needs addressing too.
(In reply to Daniel Berrangé from comment #79) > FYI, I'm finding that the RPM still fails to buld for me about 50% of the > time. This strongly suggests a makefile dependency is missing, causing > non-deterministic behaviour with parallel builds > Thanks Daniel. The error you see is due to the cbor Makefile fix for parallel build failure has not been included in github 2.22 release yet, it's only available in the tarball contained in srpm. Reason for that is such build error is only found on rawhide(with latest compiler and toolchain), not seen on stable Fedora releases, hence the integration of the fix is lagging behind. Also some other new changes/fixes are also not included in github official release yet. To address this, upstream team will create a new branch to include all the needed changes for Fedora. Later with all fixes available on new branch of linux-sgx, we'll make sure building RPM from linux-sgx github repo(+ make preparation + build.sh) work for rawhide. For the license issue of optimized_libs, it seems Fedora does not need the lib. Probably we need a dedicated download_prebuilt.sh for Fedora. We will update again once ready.
> Thanks Daniel. The error you see is due to the cbor Makefile fix for parallel build failure has not been included in github 2.22 release yet, it's only available in the tarball contained in srpm. > Reason for that is such build error is only found on rawhide(with latest compiler and toolchain), not seen on stable Fedora releases, hence the integration of the fix is lagging behind. > Also some other new changes/fixes are also not included in github official release yet. To address this, upstream team will create a new branch to include all the needed changes for Fedora. I am seeing this race condition build failure on stable Fedora releases, not only rawhide. > Also some other new changes/fixes are also not included in github official release yet. To address this, upstream team will create a new branch to include all the needed changes for Fedora. > > Later with all fixes available on new branch of linux-sgx, we'll make sure building RPM from linux-sgx github repo(+ make preparation + build.sh) work for rawhide. > For the license issue of optimized_libs, it seems Fedora does not need the lib. Probably we need a dedicated download_prebuilt.sh for Fedora. We will update again once ready. IMHO this is all missing the point. Fedora does not want to be building RPMs from special Fedora only branches of upstream that bundles Fedora. The RPM should be built from the official release *tarballs* at ie from the linux-sgx-sgx_2.22.tar.gz linked at https://github.com/intel/linux-sgx/releases/tag/sgx_2.22 If there are temporary patches needed to make it work on Fedora, add patches for those to the RPM, until those patches can be incorporated in the *main* upstream branch.
(In reply to Daniel Berrangé from comment #82) > > Also some other new changes/fixes are also not included in github official release yet. To address this, upstream team will create a new branch to include all the needed changes for Fedora. > > > > Later with all fixes available on new branch of linux-sgx, we'll make sure building RPM from linux-sgx github repo(+ make preparation + build.sh) work for rawhide. > > For the license issue of optimized_libs, it seems Fedora does not need the lib. Probably we need a dedicated download_prebuilt.sh for Fedora. We will update again once ready. > > IMHO this is all missing the point. Fedora does not want to be building RPMs > from special Fedora only branches of upstream that bundles Fedora. > > The RPM should be built from the official release *tarballs* at ie from the > linux-sgx-sgx_2.22.tar.gz linked at > https://github.com/intel/linux-sgx/releases/tag/sgx_2.22 > > If there are temporary patches needed to make it work on Fedora, add patches > for those to the RPM, until those patches can be incorporated in the *main* > upstream branch. I've taken your spec and changed it so that it uses the pristine tarballs from linux-sgx git release, along with pristine tarballs for each git submodule needed to build the SDK. The result looks like this: https://berrange.fedorapeople.org/review/linux-sgx/linux-sgx.spec https://berrange.fedorapeople.org/review/linux-sgx/linux-sgx-2.22-1.fc38.src.rpm The %prep / %build / %install stages are significantly more complicated because linux-sgx build system is quite crude and doesn't support a normal build/install approach that most apps use The important benefit of this is that we have high confidence in provenance of the source code. We also don't end up needing any of the pre-built libraries that the sgxsdk tarball bundles AFAICT, since we can replace the pre-built crypto with sgxssl + openssl. Building this way resulted in two extra libraries libtdx_tls.a and libsgx_mbedcrypto.a that sgxsdk tarball was missing. I don't know if they're desirable or not ? Also the libsgtcxx.a library is called libsgx_tcxx.a. sgxsdk seemed to take special action to rename it to libsgtcxx.a for some reason. Again I don't know if that's desirable or not ?: The %{_includedir}/sgxsdk/mbedtls/ files got installed, which was not the case with sgxsdk package too. The major unaddressed issue still is that we cannot be building a normal openssl. We must ensure that all ECC crypto is stripped out, as done in 0011-Remove-EC-curves.patch from te main fedora openssl package. BTW, I called my spec linux-sgx.spec since it is normal to name pacakges after the corresponding upstream project, but don't read too much into that. linux-sgx is a bit of an umbrella project though. In bug 2030595 you have a separate sgx-aesm-service.spec that also uses linux-sgx source. So we have two choices * Separate sgxsdk.spec and sgx-aesm-service.spec, which both use the same tarball released from linux-sgx.git, in the way you've proposed so far Or * A single linux-sgx.spec that builds absolutely everything Please take a look at my RPM proposal and let me know what you think about this approach of building from upstream source instead of the current sgxsdk source tarball. AFAICT, there should be no semantic difference between the two approaches, except for my decision to use sgxssl and no prebuilt ipp crypto
(In reply to Daniel Berrangé from comment #83) > Please take a look at my RPM proposal and let me know what you think about > this approach of building from upstream source instead of the current sgxsdk > source tarball. AFAICT, there should be no semantic difference between the > two approaches, except for my decision to use sgxssl and no prebuilt ipp > crypto Your idea and the proposal of building RPMs from upstream source makes perfect sense. It's more clean and straight forward. Thanks a lot for the updated spec, which built fine on koji along with the srpm you shared: https://koji.fedoraproject.org/koji/taskinfo?taskID=108501412 For the two choices of separated spec or single linux-sgx.spec for both sdk & aesm-service, we don't have any preferences. Which option do you recommend? The aesm-service RPMs build depends on sdk. Will it work with a single unified linux-sgx.spec to build everything? Regarding to the sgxssl instead of prebuilt ipp crypto, the missing or renamed libs are needed or not, @xiangquan could you check and comment?
(In reply to Yunying Sun from comment #84) > For the two choices of separated spec or single linux-sgx.spec for both sdk > & aesm-service, we don't have any preferences. Which option do you recommend? > The aesm-service RPMs build depends on sdk. Will it work with a single > unified linux-sgx.spec to build everything? I'm not sure I have a particularly strong recommendation either way. The default Fedora practice is usually one-upstream tarball / git repo == one downstream source package, though there have been exceptions. If we look at the maint burden going forward, then keeping the bundled 3rd party libraries in sync with desired version is a bit of a pain point, as is responding to any CVEs reproted against openssl. The burden might be reduced in this respect by having a single linux-sgx.spec On the other hand, if I look at the way the build of 'psw' interacts with the 'sdk', the 'psw' buld seems to want the sdk to be fully installed before it will build. I'm not sure how hard it will be to make it build against an uninstalled 'sdk', or an 'sdk' installed in the temporary RPM virtual root directory ? Conceptually it is nice that the 'sdk' doesn't have any pre-compiled binaries too. IOW, I can see arguments in both directions for the spec, but I'd probably tend towards a single specfile, since its the more normal Fedora practice.
(In reply to Daniel Berrangé from comment #83) > > We also don't end up needing any of the pre-built libraries that the sgxsdk > tarball bundles AFAICT, since we can replace the pre-built crypto with > sgxssl + openssl. > > Building this way resulted in two extra libraries libtdx_tls.a and > libsgx_mbedcrypto.a that sgxsdk tarball was missing. I don't know if > they're desirable or not ? These 2 libraries should be included. > Also the libsgtcxx.a library is called libsgx_tcxx.a. sgxsdk seemed to take > special action to rename it to libsgtcxx.a for some reason. Again I don't > know if that's desirable or not ?: libsgtcxx.a is a invalid library here and we can remove it. > The %{_includedir}/sgxsdk/mbedtls/ files got installed, which was not the > case with sgxsdk package too. Right. We need to put mbedtls into include folder.
Daniel, can you identify what, if anything, is being requested from Intel so that they move forward with this package inclusion?
In email discussions offlist, it was agreed that the RPM spec I provided in comment #83 is viable as a way forward. Before approving this though, I need to investigate the 'psw' part of the linux-sgx.git repository, to determine if we should include it as part of the same RPM, or create a separate RPM, and whether the needs of psw require changes to what is proposed for the sdk in #83.