[Bug 1881482] Review Request: intel-ipp-crypto-mb - Intel(R) IPP Cryptography multi-buffer library

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

 



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



--- Comment #15 from code@xxxxxxxxxxxxxxxxxx ---
> Does this library work on x86_64 platforms that do not have AVX-512 (or do not have the right collection of AVX-512 features)?

Answering this question for myself—no, it does not. I am seeking an opinion on
this: https://pagure.io/packaging-committee/issue/1044

I will wait on a resolution for that issue before attempting a full review.
However, here are the other issues I noticed:

- RPM name does not match spec file name. Rename the spec file.
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_file_naming

- Compiler build flags from the system rpm configuration are not used at all.
  Your best choice would be to change:

    %build
    cmake sources/ippcp/crypto_mb/CMakeLists.txt -B_build-crypto-mb
    cd _build-crypto-mb
    %make_build

    %install
    install -d %{buildroot}/%{_includedir}/crypto_mb
    install -m 0644 -t %{buildroot}/%{_includedir}/crypto_mb
sources/ippcp/crypto_mb/include/crypto_mb/*.h
    install -d %{buildroot}/%{_libdir}
    install -s -m 0755 _build-crypto-mb/bin/libcrypto_mb.so.%{fullversion}
%{buildroot}/%{_libdir}

  to

    %build
    pushd sources/ippcp/crypto_mb
    %cmake
    %cmake_build
    popd

    %install
    pushd sources/ippcp/crypto_mb
    install -d %{buildroot}/%{_includedir}/crypto_mb
    install -p -m 0644 -t %{buildroot}/%{_includedir}/crypto_mb
include/crypto_mb/*.h
    install -d %{buildroot}/%{_libdir}
    install -p -s -m 0755 %{_vpath_builddir}/bin/libcrypto_mb.so.%{fullversion}
%{buildroot}/%{_libdir}
    popd

  If you are targeting Fedora 32 or EPEL, use %{__cmake_builddir} in place of
  {_vpath_builddir}, or add “%undefine __cmake_in_source_build”.

  After that change, the only build flag not honored is -O2; the build system
  adds -O3. The guidelines say:

    Overriding these flags for performance optimizations (for instance, -O3
    instead of -O2) is generally discouraged. If you can present benchmarks
    that show a significant speedup for this particular code, this could be
    revisited on a case-by-case basis. Adding to and overriding or filtering
    parts of these flags is permitted if there’s a good reason to do so; the
    rationale for doing so must be documented in the specfile.

  So you should either patch this out or provide some justification for using
  -O3.

  Based on my testing, once you are respecting the usual build flags, you will
  also need to opt out of LTO. Just add “%global _lto_cflags %{nil}” somewhere
  in the spec file.

- Install commands do not preserve timestamps. Every time you use “install” to
  install a file (vs. a directory), add the -p option.

- Package is ExclusiveArch. Per
 
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures,
  must explain in a comment above the ExclusiveArch; after the package is
  approved, you must file bugs blocking the tracker bugs for all other primary
  architectures, and replace the explanation comment with links to the bugs.
  Note that it is OK to close these immediately when the reason for the
  arch-incompatibility will never be fixed or changed.


-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx




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

  Powered by Linux