https://github.com/opencryptoki/libzpc

Created attachment 1879335 [details]
SPEC file

Created attachment 1879336 [details]
Source RPM

taking for review

Hi Joerg, I see two issues
- the tar.gz archive bundled with the srpm doesn't match the one you get from github from the Source0 URL (via spectool -g libzpc.spec). Looks like the bundled version contains the complete archive of a local libzpc checkout.
- there is no %files section for the static library

and when the github source archive will be used, then "%setup -q -n %{name}" can become %autosetup (or %setup)

- the actual library is swapped between the libzpc and libzpc-devel rpms
- the value of the License tag should be just "MIT"

Hi Dan,

ok, I changed the License tag to just MIT. Will open a pr for this. 

After changing the %prep to %autosetup, I get an rpm build error, so I left this as 
"%setup -q -n %{name}" for now.

Comparing the tar.gz archives shows that I in fact included the .git and build folders, which
is probably not what you want. I rebuilt the rpm without these two folders and I'm attaching
the new rpm to this bugzilla. 

You also mentioned: "there is no %files section for the static library" 
The %files section has this entry:
%{_libdir}/%{name}.so
What else do we need here?

Thanks,
Joerg

Created attachment 1885002 [details]
Updated rpm

Updated rpm with updated spec file, without .git and build folders

(In reply to jschmidb from comment #8)
> Hi Dan,
> 
> ok, I changed the License tag to just MIT. Will open a pr for this. 
> 
> After changing the %prep to %autosetup, I get an rpm build error, so I left
> this as 
> "%setup -q -n %{name}" for now.
> 
> Comparing the tar.gz archives shows that I in fact included the .git and
> build folders, which
> is probably not what you want. I rebuilt the rpm without these two folders
> and I'm attaching
> the new rpm to this bugzilla. 

The source rpm must include the source archive downloaded from github, as referred by the Source0 tag. I guess you are building the source archive locally. The reviewed source rpm must include the source archive that matches the one from the Source0, otherwise we are possibly reviewing something completely different.
 
> You also mentioned: "there is no %files section for the static library" 
> The %files section has this entry:
> %{_libdir}/%{name}.so
> What else do we need here?

There is a %package section for the static subpackage, but no corresponding %files section. As a result the libzpc.a static library is not distributed. And as mentioned in my comment #7, the *.so file belongs to the devel subpackage and the *.so.* file into the main package.

Hi Dan,

> There is a %package section for the static subpackage, but no corresponding %files section.

I'm not completely sure what you mean here, is it like:

%files static
%{_libdir}/%{name}.so.%{soversion}*

> The source rpm must include the source archive downloaded from github, as referred by the Source0 tag.

Yes, I was in fact creating a new tar.gz from the project source.
And I see a difference between the tar.gz created locally and the one from Source0:

The locally created one contains this structure:

libzpc-1.0.0.tar
  - libzpc-1.0.0 folder
      - libzpc folder   <- missing in the tar.gz from Source0 
          - project contents ..   

The libzpc folder is missing in the tar.gz from Source0 and the rpmbuild fails. 

So does this mean the tar.gz on Source0 is incorrectly built?

Thanks for your comments via mail Dan! 

Building the rpm with "rpmbuild -bs ..." solved the build problem. I was using "rpmbuild -ba ...".
I'm attaching the new rpm.

Regarding '%files devel' versus '%files', looks like I have just swapped the two symlinks.
I'm attaching the new spec file with exchanged symlinks. 

And I added a '%files static' section with 

%files static
%{_libdir}/%{name}.a

Created attachment 1888055 [details]
Updated spec file

Created attachment 1888056 [details]
Updated rpm

Joerg, seems the spec file attached is different compared to the one in the attached source rpm.

Ah sorry, yes you are right: I made the changes in my git project folder, but did 
not copy the new spec file to rpmbuild/SPECS on my build system. 

I re-created the source rpm with the new spec file. I will create a new pull request
on github for it to include it into the libzpc lib. 

Attaching new files ...

Created attachment 1889885 [details]
Updated source rpm with new spec file

Thanks, I believe we are close now :-)

Please update the %setup command to reflect the use of the canonical (aka github) source archive which uses %{name}-%{version} format for the top level directory, I recommend to use %autosetup.

I hope the last item is the location of the pkgconfig file, which is currently installed into /usr/share/pkgconfig by the project's buildsystem. Looks like it's a valid location in addition to the classic /usr/{lib,lib64}/pkgconfig. Either location is OK, but seems there is mismatch between the CMakeLists.txt and the spec file.

OK, I changed the %setup command to:

%autosetup -q -n %{name}-%{version}

The pull request is open at:
https://github.com/opencryptoki/libzpc/pull/6

Regarding the pkgconfig file location: The previous commit "Provide spec file for rpmbuild"
changes the location to ${CMAKE_INSTALL_LIBDIR}/pkgconfig, which translates to
/usr/{local}/{lib,lib64}/pkgconfig. The spec file has %{_libdir}/pkgconfig/.
I do not see the mismatch yet ... 

Re-uploading new spec file ...

Created attachment 1890246 [details]
Updated spec file

(In reply to jschmidb from comment #19)
> OK, I changed the %setup command to:
> 
> %autosetup -q -n %{name}-%{version}
> 
> The pull request is open at:
> https://github.com/opencryptoki/libzpc/pull/6
> 
> Regarding the pkgconfig file location: The previous commit "Provide spec
> file for rpmbuild"
> changes the location to ${CMAKE_INSTALL_LIBDIR}/pkgconfig, which translates
> to
> /usr/{local}/{lib,lib64}/pkgconfig. The spec file has %{_libdir}/pkgconfig/.
> I do not see the mismatch yet ... 

Yes, but this change hasn't made it to the released libzpc archive (currently still version 1.0.0), if I see right. Looks to me it would make sense to release version 1.0.1 with the latest changes.

One reminder because I saw some test data files/URLs in the CMakeLists.txt file, the Fedora or RHEL build systems don't have access to the internet (by design), so should the build download any files on the fly, it will fail.

> Yes, but this change hasn't made it to the released libzpc archive (currently still version 1.0.0), if > I see right. Looks to me it would make sense to release version 1.0.1 with the latest changes.

Yep, makes sense. I changed the version to 1.0.1 in the spec file and CMakeLists.txt to be
consistent. But then the rpmbuild fails, because it wants to get a libzpc-1.0.1.tar.gz.
So I probably could first commit the changes on github to force rebuilding the tar.gz
and then do the rpmbuild again, or I just rename the tar.gz? But then the contained
CMakeLists.txt would not have the correct release ... hmm .. what should we do here?

I also noticed that "%autosetup -q -n %{name}-%{version}" doesn't like the -q option,
so I removed it. 

> One reminder because I saw some test data files/URLs in the CMakeLists.txt file, the Fedora or RHEL
> build systems don't have access to the internet (by design), so should the build download any files on > the fly, it will fail.

Yes, "cmake -DBUILD_TEST=ON" downloads test vectors from NIST and also downloads the Google
gtest suite. But when building without test, no internet connection is needed.

(In reply to jschmidb from comment #22)
> > Yes, but this change hasn't made it to the released libzpc archive (currently still version 1.0.0), if > I see right. Looks to me it would make sense to release version 1.0.1 with the latest changes.
> 
> Yep, makes sense. I changed the version to 1.0.1 in the spec file and
> CMakeLists.txt to be
> consistent. But then the rpmbuild fails, because it wants to get a
> libzpc-1.0.1.tar.gz.
> So I probably could first commit the changes on github to force rebuilding
> the tar.gz
> and then do the rpmbuild again, or I just rename the tar.gz? But then the
> contained
> CMakeLists.txt would not have the correct release ... hmm .. what should we
> do here?

good question, but I think the best option is to commit changes to github, make a new release (1.0.1) and use that for the next iteration in the review.
 
> I also noticed that "%autosetup -q -n %{name}-%{version}" doesn't like the
> -q option,
> so I removed it. 

plain %autosetup should be sufficient for standard source archives
 
> > One reminder because I saw some test data files/URLs in the CMakeLists.txt file, the Fedora or RHEL
> > build systems don't have access to the internet (by design), so should the build download any files on > the fly, it will fail.
> 
> Yes, "cmake -DBUILD_TEST=ON" downloads test vectors from NIST and also
> downloads the Google
> gtest suite. But when building without test, no internet connection is
> needed.

OK, no problem then

> good question, but I think the best option is to commit changes to github, make a new release (1.0.1) and use that for the next iteration in the review.

OK, I merged the pr and made a new v1.0.1. I'm attaching the final rpm (built with the v1.0.1 tar.gz) and spec file. 

> plain %autosetup should be sufficient for standard source archives

Yep, it's now just %autosetup.

Created attachment 1892213 [details]
v1.0.1 rpm

Created attachment 1892214 [details]
v1.0.1 spec file

The spec file got DOS/Win line endings (CR+LF) somehow which breaks rpmbuild :-(

Your are right, sorry! 
I'm going to re-attach the spec file with Unix-style line endings.

Created attachment 1893436 [details]
spec file with unix-style line endings

thanks for the update

But it looks to me that the current Cmake project file is able to build either a shared lib or a static lib, but not both. I suggest to remove all the sections for the static lib from the spec file, so we can finish the review. Ignore the spec file in libzpc project and make the change "locally" only.

rpmbuild -ba libzpc.spec fails with

RPM build errors:
    File not found: /home/sharkcz/rpmbuild/BUILDROOT/libzpc-1.0.1-1.fc36.s390x/usr/lib64/libzpc.a

Hmm, yes, I'm getting the same build error with rpmbuild -ba. 

I removed the static sections from the spec file as you suggested 
and the build with rpmbuild -ba now succeeds.

... re-attaching changed spec file

Created attachment 1893861 [details]
spec file without static sections

please attach the srpm as well

Created attachment 1894437 [details]
Related srpm

formal review is here, see the notes explaining OK* and BAD statuses below:

OK      source files match upstream:
    c7be71b5f4139b724ba6a8e058898056c9a7b09e  libzpc-1.0.1.tar.gz

OK      package meets naming and versioning guidelines.
OK      specfile is properly named, is cleanly written and uses macros consistently.
OK      dist tag is present.
OK      license field matches the actual license (MIT).
OK      license is open source-compatible. License text included in package.
OK      latest version is being packaged.
OK      BuildRequires are proper.
OK      compiler flags are appropriate.
OK      package builds in mock (Rawhide/s390x).
OK      debuginfo package looks complete.
OK      rpmlint is silent.
OK      final provides and requires look sane.
OK      %check is present and all tests pass.
OK      shared libraries are corectly added to the regular linker search paths.
OK      owns the directories it creates.
OK      doesn't own any directories it shouldn't.
OK      no duplicates in %files.
OK      file permissions are appropriate.
OK      no scriptlets present.
OK      code, not content.
OK      documentation is small, so no -docs subpackage is necessary.
OK      %docs are not necessary for the proper functioning of the package.
OK      headers in devel subpackage
OK      pkgconfig files in devel subpackage
OK      no libtool .la droppings.
OK      not a GUI app.

- tests require Internet access, thus they are not run during our builds

The libzpc package is APPROVED.

- Requested ACL token
- fedpkg set-pagure-token xxxxx
- fedpkg request-repo libzpc 2085377

I changed the bugzilla title to include "Review requested:", because I first got error msg
"Could not execute request_repo: Invalid title for this Bugzilla bug (no ":" present)"

Now waiting until the request is approved:
https://pagure.io/releng/fedora-scm-requests/issue/47590

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libzpc

OK, meanwhile I requested branches for f35 and f36, did a git merge rawhide, fedpkg push, and fedpkg build for both branches.

The packaging guide says I'm now supposed to close the ticket.

I'm closing with RAWHIDE as I did the build for rawhide. 

As I'm completely new to this process, please let me know if there are still todo's for me.
According to the packaging guide "New Package Process for Existing Contributors" I'm now
finishing step 12 (Close bugzilla). Not sure if I have something to do for step 13 and 14.

Closing.

Good job, thanks.

But there are few items still missing
- there is no build for rawhide (aka f38), I see only f35 and f36 builds, to be resolved with "fedpkg build" from the rawhide/main branch
- there is no f37 branch at all if I see correctly, it's the release to be GA very soon (aka "branched"), it got a separate branch from rawhide in early August

Step 13 is required to get builds for the "current" releases published, so users can actually install them. This includes F-35, F-36 and F-37.
And yes, step 14 can be skipped.

- rawhide (aka f38) is now built.

- For f37, as suggested in https://pagure.io/releng/fedora-scm-requests/issue/47711
  I did a 
  $ git checkout -b f37
  $ git push -u origin f37
  $ fedpkg build
  Here, the build failed with:
(gssapi auth failed: requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://koji.fedoraproject.org/kojihub/ssllogin)
Use following documentation to debug kerberos/gssapi auth issues. https://docs.pagure.org/koji/kerberos_gssapi_debug/
Kerberos authentication fails: unable to obtain a session (gssapi auth failed: requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://koji.fedoraproject.org/kojihub/ssllogin)
Use following documentation to debug kerberos/gssapi auth issues. https://docs.pagure.org/koji/kerberos_gssapi_debug/
Could not execute build: Could not login to https://koji.fedoraproject.org/kojihub

- Question: for Step 13 I would do for f35, f36, f37 and f38 (rawhide):
  # fedpkg build
  # fedpkg update
  Monitor the update’s status ... 
  # bodhi updates request <update_id> stable
  correct?

- to create the missing f37 branch please use "fedpkg request-branch f37"
- no need to submit an update for rawhide/f38, this is handled automagically and immediately goes into "stable", rawhide is a "rolling release"
- the "update" procedure looks correct, although the "stable request" is not necessary, because the move is done by automation by default, based on karma set by users (>=3) or by time (7 or 14 days in updates-testing)
- personally I am using the web UI (https://bodhi.fedoraproject.org/) which allows me to create updates for multiple branches by filling one form (you select multiple builds, eg. f35 + f36 + f37, and bodhi creates 3 update entries)

Thanks Dan!
 - just did a "fedpkg request-branch f37": 
   https://pagure.io/releng/fedora-scm-requests/issue/47787
 - I requested builds for f35 and f36 via the web UI and I already see
   libzpc-1.0.1-1.fc35 -> pending/testing
   libzpc-1.0.1-1.fc36 -> pending/testing
   libzpc-1.0.1-1.fc38 -> stable
So I will do the same for f37 as soon as I have it.

There's still a problem with f37:

$ git checkout f37
Switched to branch 'f37'
Your branch is up to date with 'origin/f37'.

$ git push -u origin f37
Branch 'f37' set up to track remote branch 'f37' from 'origin'.
Everything up-to-date

$ git merge f37
Already up to date.

$ fedpkg build
(gssapi auth failed: requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://koji.fedoraproject.org/kojihub/ssllogin)
Use following documentation to debug kerberos/gssapi auth issues. https://docs.pagure.org/koji/kerberos_gssapi_debug/
Kerberos authentication fails: unable to obtain a session (gssapi auth failed: requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://koji.fedoraproject.org/kojihub/ssllogin)
Use following documentation to debug kerberos/gssapi auth issues. https://docs.pagure.org/koji/kerberos_gssapi_debug/
Could not execute build: Could not login to https://koji.fedoraproject.org/kojihub

Hi Joerg, do you have a valid kerberos ticket?

Hmm, I'm not aware of any ... so most likely not.

ok, 'fkinit -u jschmidb' did the trick.
f37 is now also built and I requested an update via the web GUI.

Looks like everything is now ok in Fedora:
https://buildsys.fedoraproject.org/koji/packageinfo?packageID=36058

Please let me know if there is anything to do for me to get the packages into RHEL.
My current understanding is that is done automatically.

Thanks for the work on the Fedora package, all looks good here. There is a separate process for the inclusion in RHEL and it has already been started. Thomas will be tracking it from the IBM side.