I corrected a summary of this bug to conform to review rules.
Please provide an URL of the SPEC file. <https://github.com/intel/ipp-crypto/blob/develop/sources/ippcp/crypto_mb/tools/rpm/intel-crypto-mb.spec> is some HTML file. SRPM like wise.
Also please provide your Fedora Account System (FAS) name.
A template for a review request at <https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&format=fedora-review> has a meaning. Next time, please use the template.

Thanks for the corrections.

This was link to the SPEC github page, the direct link to the file is: <https://raw.githubusercontent.com/intel/ipp-crypto/develop/sources/ippcp/crypto_mb/tools/rpm/intel-crypto-mb.spec>
My FAS account is 'amatyuko'.

Hi Petr,

I updated the links to the spec and srpms files to the direct one.
Could you take a look on the request again?

Regards,
Andrey

URL and Source0 addresses are usable. Ok.

FATAL: Source0 archive does not match an upstream:

$ sha256sum -b intel-ipp-crypto-mb-develop.zip*
4646b73d907854af7e2237162573c5665b3af91eb1cc6079b737eca4d6a637ad *intel-ipp-crypto-mb-develop.zip
70157e23beff57cb54fb68221c2a5399d533e8fbfb2c26fdbb3c3d370905d284 *intel-ipp-crypto-mb-develop.zip.orig

The archive from a SRPM package (70157e23beff57cb54fb68221c2a5399d533e8fbfb2c26fdbb3c3d370905d284) does not match the upstream archive from the Source0 address (4646b73d907854af7e2237162573c5665b3af91eb1cc6079b737eca4d6a637ad).

The Source0 address is suspicious: It's not listed among the upstream released files <https://github.com/intel/ipp-crypto/releases>. And the URL does depend on a version of the RPM package. This does not allow to reproduce the archives for different versions.
Please use a versioned URL, preferably one that is linked from the upstream home page. If the upstream does provide release archives, use a git snapshot identified by a commit ID as documented at <https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_hosting_services>.

Few other notes to the spec file:

The %github_repo_url macro is not necessary. You can use %{url} macro for URL value and define Source0 as %{url}/archive...
The Group RPM tag is obsolete. Remove them fro the spec file.
The intel-ipp-crypto-mb-devel subpackage can be defined with "%package devel" (and "%description devel" and "%files devel"). You don't need %rpm_name macro.
The %decripion text cannot be identical to Summary text. Use more verbose description.
What's %{github_repo_url}/blob/develop/sources/ippcp/crypto_mb? Is this the real upstream home page? You should base the URL value on that.
You have to build-require "make" because you explicitly execute it (intel-crypto-mb.spec:70).
You should build-require unzip because ZIP archives are natively suppored by rpm-build.
Remove "rm -rf %{buildroot}" from %install section. Pruning the directory is done by default.
Build-require coreutils for the "install" tool (intel-crypto-mb.spec:74).
The %{_includedir}/internal path is not specific enough for your package. Please move the header files under a better-named directory.
The spec file is missing a %changelog section.

Thanks for the review, Petr.

I've updated SPEC/SRPM and fixed most of your findings.

As to the URL, you are right, it has to point to the tagged commit, so now it points to the latest release, which was out recently and contains all required changes.

The only thing that might look unusual for you is the reference to the doc in the description, that points to the doc located deep inside the source tree under %{url}.
This is the constraint of the github project organization: the crypto_mb library, that is going to be upstream-ed, now is hosted within the project for another library IPP Crypto, which is not target for upstreaming. So, that's why URL contains github home for the hosted project, and the doc refers to the inner one.

Please take a look at it again and let me know if it now looks ok. The links to updated spec/srpm are in the BZ description.

I'm glad to read you improved the package. Unfortunately, I did not find time to look at it again and now I'll be three weeks away. I'm unassigned myself from the review to give other chance to pick it up. If no new reviewer emerges in the mean time I will look at it then. I'm sorry I'm so terrible reviewer.

I try to build and here's my findings:

> BuildRequires:      openssl >= 1.1.0
> BuildRequires:      gcc >= 8.2

I think to build this library, openssl-devel and gcc-c++ are needed. Mock fails due to these two packages.

Another thing is the versioning.

> #define IFMA_VER_MAJOR  0
> #define IFMA_VER_MINOR  5
> #define IFMA_VER_REV    3

For the tag 2020u3, there is no version 0.5.3 defined in sources/ippcp/crypto_mb/include/crypto_mb/defs.h. I found the version macros in the develop branch. So will you update the version macros when you do snapshot next time? Or will 2020u3 as part of package version?

In addition, lint shows:

intel-crypto-mb.spec:73: W: setup-not-quiet
intel-ipp-crypto-mb.x86_64: E: description-line-too-long C A software crypto library optimized for Intel architecture for packet processing applications.
intel-ipp-crypto-mb.x86_64: E: description-line-too-long C It contains universal and OpenSSL compatible APIs for cryptography operations, such as:
intel-ipp-crypto-mb.x86_64: E: description-line-too-long C https://github.com/intel/ipp-crypto/blob/ippcp_2020u3/sources/ippcp/crypto_mb/Readme.md
intel-ipp-crypto-mb.x86_64: W: incoherent-version-in-changelog 0.5.3 ['0.5.3-1.fc33', '0.5.3-1']
intel-ipp-crypto-mb.x86_64: E: invalid-soname /usr/lib64/libcrypto_mb.so.0.5.3 libcrypto_mb.so
intel-ipp-crypto-mb-devel.x86_64: W: spelling-error %description -l en_US libcrypto -> cryptogram
intel-ipp-crypto-mb-devel.x86_64: E: description-line-too-long C A software crypto library optimized for Intel architecture for packet processing applications.
intel-ipp-crypto-mb-devel.x86_64: E: description-line-too-long C https://github.com/intel/ipp-crypto/blob/ippcp_2020u3/sources/ippcp/crypto_mb/Readme.md
intel-ipp-crypto-mb-devel.x86_64: W: no-documentation
2 packages and 1 specfiles checked; 6 errors, 4 warnings.
Could not execute lint: Failed to execute command.

About the "E: invalid-soname": The soname of the compiled so is:

$ objdump -p ./bin/libcrypto_mb.so | grep SONAME
  SONAME               libcrypto_mb.so

Thank you for good findings.

I've updated the description with new spec/srpms containing fixes.

The soname is fixed with the patch to 2020u3, located here:
<https://github.com/intel/ipp-crypto/raw/develop/sources/ippcp/crypto_mb/tools/rpm/SPECS/patches/0001-fix-for-soversion-in-ippcp2020u3.patch>

The only issue left is "W: spelling-error" which looks false positive.

All is good now?

Petr, Sheng, could you take a look on it again, please?

Hi Andrey Matyukov,

Thanks for the update! I see that you put 0001-fix-for-soversion-in-ippcp2020u3.patch in a subfolder "patches" and the spec file doesn't refer to the "patches" folder. Is it by design?

> Patch0:             0001-fix-for-soversion-in-ippcp2020u3.patch

The soname fix looks good to me.

Thanks for the update! Happy new year!

P.S. I can only code review the spec but I am not privileged to approve it. Sorry for that.

I'm sorry I don't have time right now. Maybe later.

The spec file link is broken. Also, the spec file name does not match the RPM name (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_file_naming).

Never mind, fedora-review was just picking up the wrong spec file link. https://raw.githubusercontent.com/intel/ipp-crypto/develop/sources/ippcp/crypto_mb/tools/rpm/SPECS/intel-crypto-mb.spec is correct.

However, the naming issue remains.

Per https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures, you also need a comment above the ExcludeArch explaining why it is required (upstream exclusively uses x86_64-specific intrinsics).

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)?

> 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.

Escalated to FESCo: https://pagure.io/fesco/issue/2569

Thanks for the feedback.

Regarding this comment:

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

Not exactly.
The library itself can work on any x86_64 platforms, but some functions (a computational ones) require specific CPU features.
Along with computational functions the library provides service functions (see cpu_features.h) that let application check if the CPU supports particular ISA to use some specific algo. Those service functions shall work regardless available ISA.

As to the fesco issue #2569, I'm not quite sure what tests are mentioned there that cannot be executed on any hardware.
It looks like it is the test/application issue, as it should perform a runtime check using provided API if the HW is supported for any algo from the library. And if not either do a fallback or gracefully exit. I think such runtime dispatching scheme is quite common when using HW dependent libraries.

Thanks for the update and the friendly correction. I had not looked closely enough, it seems, at the dispatching, and had mistaken the executables in bin/ for general-purpose tests.

I think if the previous feedback is addressed, it will be pretty easy to review this with few or no remaining quibbles.

I have updated the files according to your suggestions. Thanks for detailed comments.

Re-posting the correct/updated SPEC URL for convenience, as the fedora-review tool picks up the last such URL in the comments: https://raw.githubusercontent.com/intel/ipp-crypto/develop/sources/ippcp/crypto_mb/tools/rpm/SPECS/intel-ipp-crypto-mb.spec

Thanks for your comments and your revisions. And thanks for taking the
time to get this package into the distribution!

I found only a few small things that need to be adjusted before I can
approve the package.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

===== Issues =====

- Rather than patching from -O3 to -O2,

    -set(CMAKE_C_FLAGS_RELEASE "-O3 -DNDEBUG" CACHE STRING "" FORCE)
    +set(CMAKE_C_FLAGS_RELEASE "-O2 -DNDEBUG" CACHE STRING "" FORCE)

  just patch out the explicit optimization flag and let the CFLAGS from the
  system (which will contain -O2) prevail:

    -set(CMAKE_C_FLAGS_RELEASE "-O3 -DNDEBUG" CACHE STRING "" FORCE)
    +set(CMAKE_C_FLAGS_RELEASE "-DNDEBUG" CACHE STRING "" FORCE)

  The point is not to *override* build flags without justification.

- The patch that drops the explicit -O3 also adds

    #pragma GCC optimize ("O0")

  before several CPU feature detection routines. This part of the patch has a
  different purpose; please at least add a comment in the spec file explaining
  why it is needed. It may be cleaner to split it into a separate patch, but
  that is up to you.

- The file sources/ippcp/asm_intel64/gcm_keys_avx512.inc has a 3-clause BSD
  license in its header. Please follow
  https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios.
  The overall package license should be “ASL 2.0 and BSD”, and there must be a
  comment explaining the licensing breakdown. For example, above the License
  field:

    # The entire source code is ASL 2.0 except
    # sources/ippcp/asm_intel64/gcm_keys_avx512.inc, which is BSD
    License: ASL 2.0 and BSD

- The expanded README URL in the description is too long (80 character limit);
  unfortunately, you will have to put a line break in it somewhere. See the
  rpmlint messages and
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_summary_and_description.

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "Apache License 2.0", "*No copyright*
     Apache License 2.0", "BSD 3-clause "New" or "Revised" License". 38
     files have unknown license. Detailed output of licensecheck in
     /home/reviewer/1881482-intel-ipp-crypto-mb/licensecheck.txt

     One file is licensed BSD. See Issues, above, for what to do.

[x]: License file installed when any subpackage combination is installed.
[!]: %build honors applicable compiler flags or justifies otherwise.

     Very close; see Issues, above

[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Package is not known to require an ExcludeArch tag.

     Package does require ExcludeArch, in the form of ExclusiveArch. A proper
     justification comment is present, which must be replaced by links to
     Bugzilla bugs for all unsupported architectures after the package is
     approved
     (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures).

[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines

     (except as noted)

[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.

     See Issues, above

[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.

     No general-purpose tests provided by upstream

[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: intel-ipp-crypto-mb-1.0.1-1.fc34.x86_64.rpm
          intel-ipp-crypto-mb-devel-1.0.1-1.fc34.x86_64.rpm
          intel-ipp-crypto-mb-1.0.1-1.fc34.src.rpm
intel-ipp-crypto-mb.x86_64: E: description-line-too-long C https://github.com/intel/ipp-crypto/blob/ippcp_2020u3/sources/ippcp/crypto_mb/Readme.md
intel-ipp-crypto-mb-devel.x86_64: W: no-documentation
intel-ipp-crypto-mb.src: E: description-line-too-long C https://github.com/intel/ipp-crypto/blob/ippcp_2020u3/sources/ippcp/crypto_mb/Readme.md
3 packages and 0 specfiles checked; 2 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
intel-ipp-crypto-mb-devel.x86_64: W: no-documentation
intel-ipp-crypto-mb.x86_64: E: description-line-too-long C https://github.com/intel/ipp-crypto/blob/ippcp_2020u3/sources/ippcp/crypto_mb/Readme.md
2 packages and 0 specfiles checked; 1 errors, 1 warnings.



Source checksums
----------------
https://github.com/intel/ipp-crypto/archive/ippcp_2020u3.tar.gz#/intel-ipp-crypto-mb-ippcp_2020u3.tar.gz :
  CHECKSUM(SHA256) this package     : 0dfe8efdd85c75038027484d9343185fbd92b607a8558d65bed1caf8f5b9011c
  CHECKSUM(SHA256) upstream package : 0dfe8efdd85c75038027484d9343185fbd92b607a8558d65bed1caf8f5b9011c


Requires
--------
intel-ipp-crypto-mb (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libcrypto.so.1.1()(64bit)
    libcrypto.so.1.1(OPENSSL_1_1_0)(64bit)
    rtld(GNU_HASH)

intel-ipp-crypto-mb-devel (rpmlib, GLIBC filtered):
    intel-ipp-crypto-mb(x86-64)
    libcrypto_mb.so.1()(64bit)



Provides
--------
intel-ipp-crypto-mb:
    intel-ipp-crypto-mb
    intel-ipp-crypto-mb(x86-64)
    libcrypto_mb.so.1()(64bit)

intel-ipp-crypto-mb-devel:
    intel-ipp-crypto-mb-devel
    intel-ipp-crypto-mb-devel(x86-64)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1881482
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, C/C++, Generic
Disabled plugins: Python, Java, Haskell, PHP, Ocaml, SugarActivity, fonts, R, Perl
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

The issues were fixed with the last update (links to the spec and srpm are the same, but the patch hosting directory changed).

Good catch with the license, thanks!
We had incorrect copyright header for this file, it should have ASL 2.0 license as others, so I added yet another patch to fix it.

All patch fixes will be published in to upstream develop branch as well.

Thanks! Looks good to me; approved. Thanks again for putting in the effort to get this package into Fedora.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "Apache License 2.0", "*No copyright*
     Apache License 2.0". 38 files have unknown license. Detailed output of
     licensecheck in /home/reviewer/1881482-intel-ipp-crypto-
     mb/20210205/1881482-intel-ipp-crypto-mb/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Package is not known to require an ExcludeArch tag.

     Package does require ExcludeArch, in the form of ExclusiveArch. A proper
     justification comment is present, which must be replaced by links to
     Bugzilla bugs for all unsupported architectures after the package is
     approved
     (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures).

[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.

     No general-purpose tests provided by upstream

[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: intel-ipp-crypto-mb-1.0.1-1.fc34.x86_64.rpm
          intel-ipp-crypto-mb-devel-1.0.1-1.fc34.x86_64.rpm
          intel-ipp-crypto-mb-1.0.1-1.fc34.src.rpm
intel-ipp-crypto-mb-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
intel-ipp-crypto-mb-devel.x86_64: W: no-documentation
2 packages and 0 specfiles checked; 0 errors, 1 warnings.



Source checksums
----------------
https://github.com/intel/ipp-crypto/archive/ippcp_2020u3.tar.gz#/intel-ipp-crypto-mb-ippcp_2020u3.tar.gz :
  CHECKSUM(SHA256) this package     : 0dfe8efdd85c75038027484d9343185fbd92b607a8558d65bed1caf8f5b9011c
  CHECKSUM(SHA256) upstream package : 0dfe8efdd85c75038027484d9343185fbd92b607a8558d65bed1caf8f5b9011c


Requires
--------
intel-ipp-crypto-mb (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libcrypto.so.1.1()(64bit)
    libcrypto.so.1.1(OPENSSL_1_1_0)(64bit)
    rtld(GNU_HASH)

intel-ipp-crypto-mb-devel (rpmlib, GLIBC filtered):
    intel-ipp-crypto-mb(x86-64)
    libcrypto_mb.so.1()(64bit)



Provides
--------
intel-ipp-crypto-mb:
    intel-ipp-crypto-mb
    intel-ipp-crypto-mb(x86-64)
    libcrypto_mb.so.1()(64bit)

intel-ipp-crypto-mb-devel:
    intel-ipp-crypto-mb-devel
    intel-ipp-crypto-mb-devel(x86-64)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 1881482
Buildroot used: fedora-rawhide-x86_64
Active plugins: C/C++, Shell-api, Generic
Disabled plugins: Ocaml, Haskell, SugarActivity, fonts, Java, PHP, Ruby, R, Perl, Python
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Could you guide on the next steps for this request, please?
It looks like I cannot get it in to the repo as I'm not in packager group.
@code, can I ask you for the sponsorship?

While I’ve been doing RPM packaging for a long time, I’ve only been active in the Fedora project for about five months. I therefore need to wait another month or two before I meet the usual guidelines to be able to apply for sponsor status (https://fedoraproject.org/wiki/How_to_sponsor_a_new_contributor#Becoming_a_Fedora_Package_Collection_Sponsor). (I have done many more than five nontrivial reviews, and maintain many more than three packages.)

If you haven’t found a sponsor by May or so, please contact me again and I will consider it. Meanwhile, please examine https://fedoraproject.org/wiki/Join_the_package_collection_maintainers and https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group, which you have probably already seen.

I have also blocked FE-NEEDSPONSOR on this bug, which will help bring attention to your need for a sponsor.

Thanks, Ben.

Hi Carl,

You seemed to be reviewed our peer request for QAT Engine:
https://bugzilla.redhat.com/show_bug.cgi?id=1885495

This library (crypto_mb) is part of the QAT Engine contribution as well.

As Ben (who approved the current request) is currently unable to sponsor me to the packaging group, can I ask you to help us to complete this task and be a sponsor?


Regards,
Andrey

Andrey should be in the packager group now as sgallagh agreed to sponsor.

Thank you! Now I'm getting the fedpkg error that the review request was approved over 60 days ago.

Ben, can you please update the approval flag?

I have cycled the fedora-review flag. Hopefully that “re-approval” will work.

It worked, I was able to create a ticket in pagure (#33623). Thank you!

But it seems I still do not have proper rights - the ticket was closed with the resolution "The Bugzilla bug's review is submitted by a user that is not a packager".

How can I verify it?

https://pagure.io/releng/fedora-scm-requests/issue/33623

This is a guess, but try editing your profile (https://accounts.fedoraproject.org/user/amatyuko/) and adding your RHBZ email (andrey.matyukov). I’m not sure if that’s required.

If that doesn’t work, monitor https://pagure.io/releng/issue/10080. It seems like this might be an infrastructure bug.

Thanks for the pointers. Adding RHBZ email didn't help though.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/intel-ipp-crypto-mb

Thank you all! I was able to submit the package to rawhide, so closing the ticket.