Bug 2085444

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?