Bug 1475228 - Review Request: tpm2-abrmd - TPM2 access broker and resource manager daemon
Summary: Review Request: tpm2-abrmd - TPM2 access broker and resource manager daemon
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robert-André Mauchin 🐧
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-07-26 08:48 UTC by Yunying Sun
Modified: 2018-03-01 14:59 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-03-01 14:59:36 UTC
Type: ---
Embargoed:
zebob.m: fedora-review+


Attachments (Terms of Use)

Description Yunying Sun 2017-07-26 08:48:41 UTC
Spec URL: https://raw.githubusercontent.com/yunyings/share/master/tpm2-abrmd.spec
SRPM URL: https://github.com/yunyings/share/raw/master/tpm2-abrmd-1.1.0-1.el7.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20749893


Description: <description here>
tpm2-abrmd is a system daemon implementing the TPM2 access broker (TAB) & Resource Manager (RM) spec from the TCG.

Fedora Account System Username: yunyings

Comment 1 Yunying Sun 2017-07-26 08:51:28 UTC
Upstream repository: https://github.com/01org/tpm2-abrmd

Comment 2 Robert-André Mauchin 🐧 2017-07-26 16:14:15 UTC
Hello,

A couple of points first:

 - Systemd service files should be installed in %{_unitdir}. See https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations

 - Systemd service files also need specific scriptlets. See https://fedoraproject.org/wiki/Packaging:Scriptlets?rd=Packaging:ScriptletSnippets#Systemd

 - GCC is not needed as a BuildRequire. See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

 - why do you define %global pkg_prefix tpm2-abrmd instead of using %{name} ?
 
 - you can replace ./bootstrap with "autoreconf -vif" in %build

Comment 3 Yunying Sun 2017-07-31 08:04:30 UTC
Thanks Robert-Andre for the first review. 

With current upstream code base, tpm2-abrmd.service is installed at /usr/lib64/systemd/system/tpm2-abrmd.service, not /lib/systemd/system. 

I created a ticket for upstream: https://github.com/01org/tpm2-abrmd/issues/115,
and will update SPEC once this issue is fixed or clarified.

Comment 4 Yunying Sun 2017-07-31 09:06:25 UTC
(In reply to Robert-André Mauchin from comment #2)
> Hello,
> 
> A couple of points first:
> 
>  - Systemd service files should be installed in %{_unitdir}. See
> https://fedoraproject.org/wiki/Packaging:Systemd#Filesystem_locations
> 
Hi Robert-Andre, one question here, without upstream change, is it possible to set a different systemd service file installation path in spec file, to override the upstream default(/usr/lib64/systemd/system/)?

Comment 5 Robert-André Mauchin 🐧 2017-07-31 09:10:44 UTC
Yes, in %install just move from the file from %{buildroot}/%{_libdir}/systemd/system/tpm2-abrmd.service to %{buildroot}/%{_unitdir}

Comment 7 Yunying Sun 2017-08-01 05:45:29 UTC
%changelog
* Tue Aug 1 2017 Sun Yunying <yunying.sun> - 1.1.0-3
- Use config option with-systemdsystemunitdir to set systemd unit file location

Spec URL: https://raw.githubusercontent.com/yunyings/share/master/tpm2-abrmd.spec
SRPM URL: https://github.com/yunyings/share/raw/master/tpm2-abrmd-1.1.0-2.el7.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20938040

Comment 8 Yunying Sun 2017-08-01 05:46:53 UTC
SRPM URL should be:
https://github.com/yunyings/share/raw/master/tpm2-abrmd-1.1.0-3.el7.src.rpm

Comment 9 Robert-André Mauchin 🐧 2017-08-03 14:28:38 UTC
It looks good to me, it just need to be officially reviewed now.

Comment 10 Robert-André Mauchin 🐧 2017-08-06 08:23:09 UTC
There's nothing after your %systemd_postun, didn't you forget to add the service file there?
%systemd_postun tpm2-abrmd.service

Comment 11 Yunying Sun 2017-08-15 09:27:05 UTC
* Tue Aug 15 2017 Sun Yunying <yunying.sun> - 1.1.0-4
- Rename and relocate udev rules file to _udevrulesdir
- Update scriptlet to add service name after systemd_postrun

Spec URL: https://raw.githubusercontent.com/yunyings/share/master/tpm2-abrmd.spec
SRPM URL: https://github.com/yunyings/share/raw/master/tpm2-abrmd-1.1.0-4.el7.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21239218

Comment 12 Robert-André Mauchin 🐧 2017-08-15 10:47:48 UTC
One minor thing:
 - Source0 should be modified to https://github.com/01org/tpm2-abrmd/archive/%{version}/%{name}-%{version}.tar.gz

Otherwise, package accepted.


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

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



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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[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: "*No copyright* BSD (2 clause)", "BSD (2 clause)", "Unknown or
     generated". 26 files have unknown license. Detailed output of
     licensecheck in /home/bob/packaging/review/tpm2-abrmd/review-
     tpm2-abrmd/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib/udev,
     /usr/lib/udev/rules.d
[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.
[x]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 2 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 does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[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]: %config files are marked noreplace or the reason is justified.
[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 use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[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]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tpm2
     -abrmd-debuginfo , tpm2-abrmd-devel
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: 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.
[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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[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 debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[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]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: tpm2-abrmd-1.1.0-4.fc27.x86_64.rpm
          tpm2-abrmd-debuginfo-1.1.0-4.fc27.x86_64.rpm
          tpm2-abrmd-devel-1.1.0-4.fc27.x86_64.rpm
          tpm2-abrmd-1.1.0-4.fc27.src.rpm
tpm2-abrmd.x86_64: W: shared-lib-calls-exit /usr/lib64/libtcti-tabrmd.so.0.0.0 exit.5
tpm2-abrmd.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
tpm2-abrmd.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
tpm2-abrmd.x86_64: W: one-line-command-in-%postun /sbin/ldconfig
tpm2-abrmd-devel.x86_64: W: spelling-error Summary(en_US) config -> con fig, con-fig, configure
tpm2-abrmd-devel.x86_64: W: spelling-error %description -l en_US config -> con fig, con-fig, configure
tpm2-abrmd-devel.x86_64: W: only-non-binary-in-usr-lib
tpm2-abrmd-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 8 warnings.

Comment 13 Yunying Sun 2017-08-16 03:14:06 UTC
Thanks Robert-Andre for the review and approve. 

I updated SPEC with the source0 url in your comment.
* Wed Aug 16 2017 Sun Yunying <yunying.sun> - 1.1.0-5
- Updated source0 URL to fix rpmlint warnings

Spec URL: https://raw.githubusercontent.com/yunyings/share/master/tpm2-abrmd.spec
SRPM URL: https://github.com/yunyings/share/raw/master/tpm2-abrmd-1.1.0-5.el7.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21253304

Comment 14 Yunying Sun 2017-08-16 07:53:35 UTC
New package request created: https://pagure.io/releng/fedora-scm-requests/issue/61

Comment 15 Gwyn Ciesla 2017-08-16 12:08:40 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/tpm2-abrmd

Comment 16 Gwyn Ciesla 2017-08-16 12:09:04 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/tpm2-abrmd

Comment 17 Yunying Sun 2017-08-17 02:48:46 UTC
$fedpkg clone rpms/tpm2-abrmd
/usr/lib/python2.7/site-packages/fedora/client/bodhi.py:48: DeprecationWarning: fedora.client.bodhi has been deprecated. Please use bodhi.client.bindings instead.
  DeprecationWarning)
Cloning into 'tpm2-abrmd'...
remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (3/3), done.

$cd tpm2-abrmd/
$fedpkg import ~/rpmbuild/SRPMS/tpm2-abrmd-1.1.0-5.el7.src.rpm 
$git add .
$git commit -m "Initial import (#1475228)"

$fedpkg push
/usr/lib/python2.7/site-packages/fedora/client/bodhi.py:48: DeprecationWarning: fedora.client.bodhi has been deprecated. Please use bodhi.client.bindings instead.
  DeprecationWarning)
FATAL: W any rpms/tpm2-abrmd yunyings DENIED by fallthru
(or you mis-spelled the reponame)
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
Could not execute push: Command '['git', 'push']' returned non-zero exit status 128

Seeing at https://src.fedoraproject.org/rpms/tpm2-abrmd/settings, I'm the admin of this repo: yunyings (main admin) 

But koji list-pkgs shows Owner of this package is "releng":
[test@SKLS02 tpm2-abrmd]$ koji list-pkgs --package=tpm2-abrmd --show-blocked
Package                 Tag                     Extra Arches     Owner          
----------------------- ----------------------- ---------------- ---------------
tpm2-abrmd              f27                                      releng 

Not sure if that's problem or not. 
Does anyone knows how to fix this push error?

Comment 18 Robert-André Mauchin 🐧 2017-08-17 08:48:13 UTC
Could you retry now to see if it's fixed?

Otherwise manifest yourself on this bug report https://pagure.io/fedora-infrastructure/issue/6236

Comment 19 Yunying Sun 2017-08-17 09:04:06 UTC
Just tried. Still failed, but with different error.

[test@SKLS02 tpm2-abrmd]$ fedpkg push 
/usr/lib/python2.7/site-packages/fedora/client/bodhi.py:48: DeprecationWarning: fedora.client.bodhi has been deprecated. Please use bodhi.client.bindings instead.
  DeprecationWarning)
Counting objects: 3, done.
Writing objects: 100% (2/2), 210 bytes | 0 bytes/s, done.
Total 2 (delta 0), reused 0 (delta 0)
remote: Traceback (most recent call last):
remote:   File "./hooks/post-receive.default", line 19, in <module>
remote:     import pagure  # noqa: E402
remote:   File "/usr/lib/python2.7/site-packages/pagure/__init__.py", line 62, in <module>
remote:     APP.config.from_envvar('PAGURE_CONFIG')
remote:   File "/usr/lib/python2.7/site-packages/flask/config.py", line 108, in from_envvar
remote:     return self.from_pyfile(rv, silent=silent)
remote:   File "/usr/lib/python2.7/site-packages/flask/config.py", line 128, in from_pyfile
remote:     with open(filename) as config_file:
remote: IOError: [Errno 13] Unable to load configuration file (Permission denied): '/etc/pagure/pagure.cfg'
remote: Hook ./hooks/post-receive.default failed with error code 1
To ssh://yunyings.org/rpms/tpm2-abrmd
   4665bdc..5a800a2  master -> master

Comment 20 Robert-André Mauchin 🐧 2017-08-17 09:12:47 UTC
I don't think this error is important, if it says it pushed master, it ushed master.

However I see a commit here https://src.fedoraproject.org/rpms/tpm2-abrmd/c/5a800a2346ea47b553fb94ccac1dda9f33403abc?branch=master
But it doesn't contain the sources and spec files. Are you sure you didn't forget to do "fedpkg import PATH_TO_SRPM first"?

Comment 21 Yunying Sun 2017-08-17 09:23:07 UTC
Thanks Robert-Andre. You are right, I made an empty commit by mistake and pushed.
Just reverted the commit, and pushed a new one with valid files.

Comment 22 Yunying Sun 2017-08-17 09:30:41 UTC
But "fedpkg build"(https://koji.fedoraproject.org/koji/taskinfo?taskID=21277310) failed with:
FAILED: BuildError: package tpm2-abrmd not in list for tag f28-pending

Is there something I should do to fix this?

Comment 23 Robert-André Mauchin 🐧 2017-08-17 09:36:27 UTC
As far as I know:

« The script [that picks up new packages] does currently only run once per hour and takes about 30min, so you could have a 1.5 hour gap there between a new package being added and it getting synced to koji. »


So you should wait a bit 1:30/2h after pushing your new package. If the problem still persists, please mention it on https://pagure.io/fedora-infrastructure/issue/6236

Comment 24 Yunying Sun 2017-08-17 09:45:51 UTC
Understood. I will try koji build tomorrow. Thanks.

BTW, according to https://pagure.io/fedora-infrastructure/issue/6235, seems the "IOError: [Errno 13] Unable to load configuration file (Permission denied)" is an infrastructure issue which not yet fixed.

Comment 25 Yunying Sun 2017-08-18 02:19:17 UTC
Koji rawhide build completed for tpm2-abrmd-1.1.0-5:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21292902

Comment 26 Yunying Sun 2017-08-18 03:19:16 UTC
f27 has tpm2-tss 1.1.0, which does not provide resource manager daemon anymore.
tpm2-abrmd is the replacing daemon, which is required when using many tpm2 userspace tools like those tpm2_*** provided by tpm2-tools. Therefore tpm2-abrmd is needed for f27 as well. 

I did a tpm2-abrmd-1.1.0-5 koji build for f27:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21293091

But "fedpkg update" failed for f27 with error:
[test@SKLS02 tpm2-abrmd]$ git branch
* f27
  master
[test@SKLS02 tpm2-abrmd]$ fedpkg update
Invalid tag: tpm2-abrmd-1.1.0-5.fc27 not tagged with any of the following tags [u'f22-updates-candidate', u'dist-6E-epel-testing-candidate', u'f21-updates-candidate', u'f24-updates-candidate', u'epel7-testing-candidate', u'f25-updates-candidate', u'dist-5E-epel-testing-candidate', u'f23-updates-candidate', u'f26-updates-candidate']
Unable to determine release from build: tpm2-abrmd-1.1.0-5.fc27

Seems f27 is at early branched state before Bodhi activation point:
https://fedoraproject.org/wiki/Releases/27/Schedule

According to https://fedoraproject.org/wiki/Package_update_HOWTO#Rawhide and early Branched , suppose no "fedpkg update" needed and tpm2-abrmd-1.1.0-5 will be included into f27 automatically.

Comment 27 Javier Martinez Canillas 2018-03-01 14:59:36 UTC
Yunying,

We can close this bug now that the tpm2-abrmd is already included in Fedora 27.


Note You need to log in before you can comment on or make changes to this bug.