Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1282903 - Review Request: oci-register-machine - Golang binary for registering OCI containers with systemd-machined
Summary: Review Request: oci-register-machine - Golang binary for registering OCI cont...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nalin Dahyabhai
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2015-11-17 18:48 UTC by Sally
Modified: 2016-08-14 15:58 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-08-14 15:58:27 UTC
Type: ---
Embargoed:
nalin: fedora-review+

Attachments (Terms of Use)
update spec file (3.03 KB, patch)
2015-11-20 10:01 UTC, Jan Chaloupka 		no flags Details | Diff
Update license and %files section (4.32 KB, patch)
2015-11-20 11:46 UTC, Jan Chaloupka 		no flags Details | Diff
Show Obsolete (1) View All

Description Sally 2015-11-17 18:48:48 UTC 
Spec URL:  https://github.com/sallyom/oci-register-stuff/blob/master/oci-register-machine-hook.spec

SRPM URL: https://github.com/sallyom/oci-register-stuff/blob/master/golang-github-sallyom-Register-0-0.1.git79c2239.fc23.src.rpm

Description: oci-register-machine-hook is a Golang binary for OCI. If you add it to the runc json data as a hook, the binary will be executed after the container process is created but before it is started, with a prestart flag. When the container exits oci-register-machine-hook will be executed after the container exits but before it is destroyed, with the poststop flag. Docker will execute oci-register-machine-hook as a container hook when it is installed in the $HOOKSDIR directory.

After starting an OCI container, the container will be listed with the machinectl. The machine name will be the container ID. Machinectl will then be able to manage the container. When container exits, oci-register-machine-hook will remove the instance from machinectl.

Fedora Account System Username: sallyom
Comment 1 Jan Chaloupka 2015-11-18 12:43:49 UTC 
Hi Sally,

can you regenerate the spec file with the latest build for f23 [1]?. I have built it today. It contains some updates of generated spec file, making the spec file more clean. Karmas on it would be appreciated [2].

'gofed repo2spec --detect https://github.com/sallyom/Register --commit 79c22398b30da927f4fd58aa45b5155bd859d34e --with-build' will do the magic.

In general:
- it is always good to provide devel subpackage so other can import your source codes. Even if the current devel subpackage would be empty, it is scanned by other tools.
- any [Build]Required golang package should be a virtual provide, not physical one, i.e. use golang(github.com/godbus/dbus) instead of golang-github-godbus-dbus-devel. In future the name of the golang-github-godbus-dbus package can change. Virtual provides does not change unless the imported package is removed.

Can you provide LICENSE file for https://github.com/sallyom/Register? Without that the package can not be packaged in Fedora.

[1] http://koji.fedoraproject.org/koji/buildinfo?buildID=700023
[2] https://bodhi.fedoraproject.org/updates/FEDORA-2015-bd2922d501
Comment 2 Sally 2015-11-19 19:37:25 UTC 
- added a LICENSE file to https://github.com/sallyom/Register
- updated spec here: https://github.com/sallyom/oci-register-stuff

thanks
Comment 3 Jan Chaloupka 2015-11-20 10:01:40 UTC 
Created attachment 1097089 [details]
update spec file
Comment 4 Jan Chaloupka 2015-11-20 11:46:39 UTC 
Created attachment 1097134 [details]
Update license and %files section
Comment 5 Jan Chaloupka 2015-11-20 11:49:50 UTC 
Patch:
- updated commit to the latest on (git commit --amend has changed the hash)
- updated ExclusiveArch tag
- removed unnecessary commented commands
- added LICENSE under %license tag
- wrapped BR of the main package with "with_bundled" macro (BR is not needed if the package is built from bundled deps)
- add unit-test-devel subpackage, disable check and unit-test as there are no tests atm

Summary(after applying the patch):
- devel subpackage presented (even if one file, it contains a dependency on dbus which is discovered during scan and used when building dependency graph for all golang projects packaged in Fedora)
- unit-test-devel subpackage presented (usefull for CI testing, atm there are no test files. However, it is better to disable generating the unit-test-devel subpackage (and enable it later) than to remove it.
- license ok
- complete list of build-time dependencies
- debuginfo genereated
Comment 6 Sally 2015-11-24 19:48:50 UTC 
- updated spec, src.rpm here: https://github.com/sallyom/oci-register-stuff
Comment 7 Nalin Dahyabhai 2015-11-25 00:00:57 UTC 
I can sponsor.  Package is in pretty good shape.

General notes:
Line 127 of the .spec file's mention of "devel.file-list" should probably be changed to "unit-test-devel.file-list".
I needed to change the definition of "with_unit_test" on line 6 from 1 to 0 to get the build to succeed in mock because the package doesn't contain tests.
The Summary: for the devel subpackage could use tweaking, since the subpackage doesn't include the binary.
Does the devel subpackage benefit from including the non-license docs?
The package %description could be expanded into something longer than the package summary, since the description can be multiple lines.
The empty %if/%endif at line 59 can probably just be removed.

Results from fedora-review:

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

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


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

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
     - If you haven't yet, please check with Dan if GPLv2+ is the right license
       for this.  Most of the OpenContainers work carries the ASL 2.0 license.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
     - Except for debuginfo, of course.
[ ]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/libexec/docker/hooks.d
     - I expect you'll need to "Requires: runc" if it ends up providing the
       /usr/libexec/docker/hooks.d directory, or the docker package, unless
       this is going to be the only thing we package that lives in that
       directory, in which case this package should also include the
       directory in its %files list (as "%dir %{_libexecdir}/docker/hooks.d").
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/gocode/src,
     /usr/share/gocode/src/github.com, /usr/libexec/docker,
     /usr/share/gocode, /usr/libexec/docker/hooks.d
     - These should be owned by other packages.
[-]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
     - Don't forget to correct the release part of the comment once it's
       imported.
[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).
[ ]: Package is named according to the Package Naming Guidelines.
     - The main package, which provides a binary, might need to be renamed to
       oci-register-machine-hook to conform to
       https://fedoraproject.org/wiki/PackagingDrafts/Go if the fact that it's
       buried under /usr/libexec doesn't exempt it from the guidelines'
       recommendation.
[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.
[ ]: Spec file is legible and written in American English.
     - Change "source codes" to "source code".
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: 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 20480 bytes in 4 files.
[!]: Package complies to the Packaging Guidelines
     - Spec file name must match the spec package %{name}.  (Also listed as
       its own requirement below.)
     - Guidelines recommend installing man pages in uncompressed form and
       listing them in the %files section with a "*" on the end to pick them
       up after rpm-build has applied whatever form of compression is
       preferred.
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[ ]: Rpmlint is run on all rpms the build produces.
     - The man page unnecessarily has the executable bit set on it.
     - The package description needs to be line-wrapped.
[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 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]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[ ]: Permissions on files are set properly.
     - The man page needs the execute bit cleared; everything else is fine.
[-]: 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.
[!]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
     - This is from the packaging guidelines.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

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

Generic:
[!]: Uses parallel make %{?_smp_mflags} macro.
     - Please go ahead and change the "make" invocation in the %build section
       to "make %{?_smp_mflags}".
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[!]: Final provides and requires are sane (see attachments).
     - Is the devel package supposed to "Provide:
       golang(github.com/sallyom/Register) = = %{version}-%{release}"?
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in golang-
     github-sallyom-Register-devel , golang-github-sallyom-Register-
     debuginfo
     - The devel package doesn't actually require a matching version of the
       main package, and debuginfo doesn't have this requirement.
[ ]: Package functions as described.
     - When run without any arguments, the tool panics.  Otherwise I expect
       it's probably okay.
[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.
[-]: Packages should try to preserve timestamps of original installed
     files.
     - The package compresses the man page and "install"s the new compressed
       copy without using the -p flag, so there would be some discrepancies
       between arches there.  If it changes to install an uncompressed man
       page, it should use "install"'s -p flag.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: 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:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[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.


Installation errors
-------------------
INFO: mock.py version 1.2.13 starting (python version = 3.4.3)...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled ccache
Mock Version: 1.2.13
INFO: Mock Version: 1.2.13
Finish: chroot init
INFO: installing package(s): /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-0-0.1.gitffa5786.fc24.x86_64.rpm /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-devel-0-0.1.gitffa5786.fc24.noarch.rpm /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-debuginfo-0-0.1.gitffa5786.fc24.x86_64.rpm /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-debuginfo-0-0.1.gitffa5786.fc24.x86_64.rpm
ERROR: Command failed. See logs for output.
 # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 24 install /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-0-0.1.gitffa5786.fc24.x86_64.rpm /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-devel-0-0.1.gitffa5786.fc24.noarch.rpm /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-debuginfo-0-0.1.gitffa5786.fc24.x86_64.rpm /tmp/tmp/review-golang-github-sallyom-Register/results/golang-github-sallyom-Register-debuginfo-0-0.1.gitffa5786.fc24.x86_64.rpm --setopt=tsflags=nocontexts


Rpmlint
-------
Checking: golang-github-sallyom-Register-0-0.1.gitffa5786.fc24.x86_64.rpm
          golang-github-sallyom-Register-devel-0-0.1.gitffa5786.fc24.noarch.rpm
          golang-github-sallyom-Register-debuginfo-0-0.1.gitffa5786.fc24.x86_64.rpm
          golang-github-sallyom-Register-0-0.1.gitffa5786.fc24.src.rpm
golang-github-sallyom-Register.x86_64: W: spelling-error Summary(en_US) systemd -> systems, system, system d
golang-github-sallyom-Register.x86_64: W: spelling-error %description -l en_US systemd -> systems, system, system d
golang-github-sallyom-Register.x86_64: W: incoherent-version-in-changelog 0-0.1.git79c2239 ['0-0.1.gitffa5786.fc24', '0-0.1.gitffa5786']
golang-github-sallyom-Register.x86_64: W: spurious-executable-perm /usr/share/man/man1/oci-register-machine-hook.1.gz
golang-github-sallyom-Register-devel.noarch: W: spelling-error Summary(en_US) systemd -> systems, system, system d
golang-github-sallyom-Register-devel.noarch: W: spelling-error %description -l en_US systemd -> systems, system, system d
golang-github-sallyom-Register-debuginfo.x86_64: E: description-line-too-long C This package provides debug information for package golang-github-sallyom-Register.
golang-github-sallyom-Register.src: W: spelling-error Summary(en_US) systemd -> systems, system, system d
golang-github-sallyom-Register.src: W: spelling-error %description -l en_US systemd -> systems, system, system d
4 packages and 0 specfiles checked; 1 errors, 8 warnings.




Requires
--------
golang-github-sallyom-Register (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libpthread.so.0()(64bit)

golang-github-sallyom-Register-devel (rpmlib, GLIBC filtered):

golang-github-sallyom-Register-debuginfo (rpmlib, GLIBC filtered):



Provides
--------
golang-github-sallyom-Register:
    golang-github-sallyom-Register
    golang-github-sallyom-Register(x86-64)

golang-github-sallyom-Register-devel:
    golang-github-sallyom-Register-devel

golang-github-sallyom-Register-debuginfo:
    golang-github-sallyom-Register-debuginfo
    golang-github-sallyom-Register-debuginfo(x86-64)



Source checksums
----------------
https://github.com/sallyom/Register/archive/ffa57865cfdaab4e0fe68708004778d0e5ea916d/Register-ffa5786.tar.gz :
  CHECKSUM(SHA256) this package     : 24d2417fa2d74a6d704d59cf30da6325073460a3a3fe83f98692365d0a86c9dd
  CHECKSUM(SHA256) upstream package : 24d2417fa2d74a6d704d59cf30da6325073460a3a3fe83f98692365d0a86c9dd


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review -n golang-github-sallyom-Register
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Comment 8 Jan Chaloupka 2015-11-25 09:00:51 UTC 
> General notes:
> Line 127 of the .spec file's mention of "devel.file-list" should probably be
> changed to "unit-test-devel.file-list".

It is better to let the directory be owned by devel subpackage even if it contains files for unit-test-devel. Directories usually contain both devel and test files which would result in a directory owned be two different packages. The idea here is to let devel to own all directories up to some directories that are obviously only for testing, such as testdata, test_files, ....

> I needed to change the definition of "with_unit_test" on line 6 from 1 to 0 to
> get the build to succeed in mock because the package doesn't contain tests.

Agree, the patch disables with_check and with_unit_test in the if branch. The current spec files disables both in else branch.

> Does the devel subpackage benefit from including the non-license docs?

Yes and no. Some *.md files can contain additional information for developer. But usually it helps to get a better picture of what the project is about and used for. As devel subpackages are used only for building atm and not for development, it does not. However, it does not hurt.

> The empty %if/%endif at line 59 can probably just be removed.

It is for future use. Once the project extends and new dependencies (not for main packages) are imported, it will be useful. The idea is to "Keep it there and just add BuildRequires later so you know where it belongs and don't have to write it down completely". The same holds for unit-test-devel. It could be deleted. But later, it will take more time to put it back.
Comment 9 Sally 2015-12-11 16:58:10 UTC 
https://github.com/sallyom/oci-register-stuff

Spec file should be looking better, now.  Thanks!
Comment 10 Jan Chaloupka 2016-01-11 13:09:04 UTC 
Looks ok to me. It conforms to the current golang packaging draft.
Comment 11 Daniel Walsh 2016-01-27 15:03:26 UTC 
I have moved this specfile into projectatomic and renamed package to oci-register-machine.

https://github.com/projectatomic/oci-register-machine

Only modifications to the spec file have been name changes.
Comment 12 Jan Chaloupka 2016-01-28 09:23:25 UTC 
Makefile contains 'go get' command:
go get github.com/cpuguy83/go-md2man

It is against packaging policies. It must by patched in the package. The project is already built in the distribution as golang-github-cpuguy83-go-md2man package. It provides go-md2man binary, so "Requires: go-md2man" will install the package.

At the same time if you use Makefile
- the package can not be built with debug info support
- the package can be built only for architectures with golang compiler, no with gcc-go
unless you patch the Makefile. Would recommend to move content of Makefile into the spec instead of running make. However, not blocker for the review.

If possible, create Godeps.json file for the project with commit of github.com/godbus/dbus used. I.e.

{
	"ImportPath": "github.com/coreos/etcd",
	"GoVersion": "go1.5.1",
	"Packages": [
		"./..."
	],
	"Deps": [
		{
			"ImportPath": "github.com/godbus/dbus",
			"Rev": "COMMIT"
		}
	]
}

so we can validate the dependency is provided by the distribution and up-to-date.
Comment 13 Daniel Walsh 2016-01-28 15:27:04 UTC 
BuildRequires: golang-github-cpuguy83-go-md2man
Comment 14 Sally 2016-01-28 20:49:04 UTC 
The Makefile and spec in the new upstream url were not current :)
I updated the files and uploaded a new srpm for review.

new upstream url: https://github.com/projectatomic/oci-register-machine
review repo with updated files: https://github.com/sallyom/oci-register-stuff

Thanks
Comment 15 Nalin Dahyabhai 2016-01-29 00:49:03 UTC 
fedora-review hit its own bug #1264803, but a manual check shows that the package installation succeeds.  We've got some issues around being able to use the -devel package's contents when building other packages; the rest of the notes cover things which I think will be pretty straightforward to tweak.  Please let me know if I need to elaborate on anything or provide assistance.

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

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


Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://fedoraproject.org/wiki/Packaging:Guidelines

  Manual check succeeds, so ignore this issue.

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

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 is ASL 2.0, links only with BSD golang-github-godbus-dbus-devel

[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". 5 files have unknown license. Detailed
     output of licensecheck in /misc/register/review-oci-register-
     machine/licensecheck.txt

     ASL 2.0

[x]: License file installed when any subpackage combination is installed.
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/gocode/src,
     /usr/libexec/docker, /usr/share/gocode,
     /usr/share/gocode/src/github.com

     Might need to add Requires: on docker for /usr/libexec/docker.
     /usr/share/gocode/src and /usr/share/gocode/src/github.com are owned by golang, which should be fine.
     /usr/libexec/docker/hooks.d probably needs to be provided by multiple packages if docker isn't going to.
     /usr/share/gocode/src/github.com/projectatomic probably needs to be provided by multiple packages, so it's fine.

[!]: %build honors applicable compiler flags or justifies otherwise.

     The Makefile doesn't pick up $LDFLAGS from the environment.  Per /usr/lib/rpm/macros.d/macros.golang-compiler, Fedora 24 defines gobuild() as:
     go build -compiler gc -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n')" -a -v -x
     If the makefile's going to run the compiler, it may want to pick up some of this.

[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.

     Version tag on the changelog entry doesn't match the version of the package, but that's easily fixed.

[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[ ]: Development files must be in a -devel package

     The source for the man page isn't really devel documentation; I think it can be dropped from the -devel package.

     I can't import "github.com/projectatomic/oci-register-machine" with the devel package installed -- I get a 'import "github.com/projectatomic/oci-register-machine" is a program, not an importable package' error.  Were the reusable bits supposed to be broken out into an oci-register-machine package and called from main()?  If so, then the compiler won't accept "oci-register-machine" as a package name, and this will affect the Provides: in the devel package.

[x]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory
     names).

     The makefile hard-codes the location where the man page is installed.  If the Makefile defines a MANDIR variable, we can override it to follow %{_mandir} during the %install phase.

[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.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.

     It could stand to have a longer %description, but otherwise it's fine.

[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: 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 20480 bytes in 4 files.
[x]: Package complies to the Packaging Guidelines
[ ]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.

     If %{with_bundled} is set to 1, it'll lose the BuildRequires: on golang(github.com/godbus/dbus), and it isn't carrying a vendored copy, so the build would fail.

[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]: 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]: 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).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in oci-
     register-machine-devel , oci-register-machine-debuginfo
[x]: 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.
[-]: Packages should try to preserve timestamps of original installed
     files.

     Installed files are generated at build-time.

[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Uses parallel make %{?_smp_mflags} macro.
[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:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[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.


Installation errors
-------------------
INFO: mock.py version 1.2.14 starting (python version = 3.5.1)...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled ccache
Mock Version: 1.2.14
INFO: Mock Version: 1.2.14
Finish: chroot init
INFO: installing package(s): /misc/register/review-oci-register-machine/results/oci-register-machine-0-0.1.git1e10ae0.fc24.x86_64.rpm /misc/register/review-oci-register-machine/results/oci-register-machine-devel-0-0.1.git1e10ae0.fc24.noarch.rpm /misc/register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git1e10ae0.fc24.x86_64.rpm /misc/register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git1e10ae0.fc24.x86_64.rpm
ERROR: Command failed. See logs for output.
 # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 24 --setopt=deltarpm=false install /misc/register/review-oci-register-machine/results/oci-register-machine-0-0.1.git1e10ae0.fc24.x86_64.rpm /misc/register/review-oci-register-machine/results/oci-register-machine-devel-0-0.1.git1e10ae0.fc24.noarch.rpm /misc/register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git1e10ae0.fc24.x86_64.rpm /misc/register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git1e10ae0.fc24.x86_64.rpm --setopt=tsflags=nocontexts


Rpmlint
-------
Checking: oci-register-machine-0-0.1.git1e10ae0.fc24.x86_64.rpm
          oci-register-machine-devel-0-0.1.git1e10ae0.fc24.noarch.rpm
          oci-register-machine-debuginfo-0-0.1.git1e10ae0.fc24.x86_64.rpm
          oci-register-machine-0-0.1.git1e10ae0.fc24.src.rpm
oci-register-machine.x86_64: W: spelling-error Summary(en_US) Golang -> Golan, Golan g, Angolan
oci-register-machine.x86_64: W: spelling-error Summary(en_US) systemd -> systems, system, system d
oci-register-machine.x86_64: W: spelling-error %description -l en_US Golang -> Golan, Golan g, Angolan
oci-register-machine.x86_64: W: spelling-error %description -l en_US systemd -> systems, system, system d
oci-register-machine.x86_64: W: incoherent-version-in-changelog 0-0.1.git6863 ['0-0.1.git1e10ae0.fc24', '0-0.1.git1e10ae0']
oci-register-machine-devel.noarch: W: spelling-error Summary(en_US) Golang -> Golan, Golan g, Angolan
oci-register-machine-devel.noarch: W: spelling-error Summary(en_US) systemd -> systems, system, system d
oci-register-machine-devel.noarch: W: spelling-error %description -l en_US Golang -> Golan, Golan g, Angolan
oci-register-machine-devel.noarch: W: spelling-error %description -l en_US systemd -> systems, system, system d
oci-register-machine-devel.noarch: W: spelling-error %description -l en_US github -> git hub, git-hub, GitHub
oci-register-machine.src: W: spelling-error Summary(en_US) Golang -> Golan, Golan g, Angolan
oci-register-machine.src: W: spelling-error Summary(en_US) systemd -> systems, system, system d
oci-register-machine.src: W: spelling-error %description -l en_US Golang -> Golan, Golan g, Angolan
oci-register-machine.src: W: spelling-error %description -l en_US systemd -> systems, system, system d
4 packages and 0 specfiles checked; 0 errors, 14 warnings.




Requires
--------
oci-register-machine-devel (rpmlib, GLIBC filtered):

oci-register-machine (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libpthread.so.0()(64bit)

oci-register-machine-debuginfo (rpmlib, GLIBC filtered):



Provides
--------
oci-register-machine-devel:
    golang(github.com/projectatomic/oci-register-machine)
    oci-register-machine-devel

oci-register-machine:
    oci-register-machine
    oci-register-machine(x86-64)

oci-register-machine-debuginfo:
    oci-register-machine-debuginfo
    oci-register-machine-debuginfo(x86-64)



Source checksums
----------------
https://github.com/projectatomic/oci-register-machine/archive/1e10ae06d1849c730a9e8eb9f9b7fdafe8d68b6e/oci-register-machine-1e10ae0.tar.gz :
  CHECKSUM(SHA256) this package     : fa72b92306f8a0adf8106bd648228879df7d0a3aeecc21a76f053c5472640333
  CHECKSUM(SHA256) upstream package : fa72b92306f8a0adf8106bd648228879df7d0a3aeecc21a76f053c5472640333


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review -n oci-register-machine
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Comment 16 Jan Chaloupka 2016-01-29 07:27:31 UTC 
> I can't import "github.com/projectatomic/oci-register-machine" with the devel
> package installed -- I get a 'import "github.com/projectatomic/oci-register-machine"
> is a program, not an importable package' error.  Were the reusable bits
> supposed to be broken out into an oci-register-machine package and called
> from main()?  If so, then the compiler won't accept "oci-register-machine"
> as a package name, and this will affect the Provides: in the devel package.

Sally, if the devel subpackage will contain only files with 'package main' directive, nothing from the subpackage can not be imported. Either set with_devel to 0 or break oci-register-machine.go into to files so there is at least one file with 'package oci-register-machine' directive.

> If %{with_bundled} is set to 1, it'll lose the BuildRequires: on
> golang(github.com/godbus/dbus), and it isn't carrying a vendored copy,
> so the build would fail.

This is useful for future use when vendor or Godeps directory are present. At the current state it does not make sense to set with_bundled to 1.
Comment 17 Daniel Walsh 2016-01-29 13:44:30 UTC 
Yes this should not contain a devel package, this is only a plugin executable.
Comment 18 Daniel Walsh 2016-02-17 20:04:14 UTC 
Dropped devel package and updated code.  We are not setting with_devel or with_bundled.
Comment 19 Daniel Walsh 2016-02-17 22:41:54 UTC 
Also fixed the LDFLAGS problem, and moved content to /usr/libexec/oci/hooks.d

If this passes review please assign this package to me, if Sally is not an approved packager at this point.
Comment 20 Nalin Dahyabhai 2016-02-19 19:39:05 UTC 
Package tested by pulling .spec file from upstream repository, updating its
%{commit} value to 157f2efcb85ebe4d45ec8df4d1002e8200ca4d51, running
 spectool -g
to downoad matching sources, and proceeding from there.

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

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


Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://fedoraproject.org/wiki/Packaging:Guidelines
  * Verified manually.  The test failed because it attempted to install the same debuginfo package twice, and dnf didn't like that.


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

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.
     Packages is ASL 2.0, depends only on a BSD-licensed package.
[x]: License field in the package spec file matches the actual license.
     Package's LICENSE file says ASL 2.0, and the .spec file agrees.
[x]: License file installed when any subpackage combination is installed.
     * There's only one binary package.
[ ]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/libexec/oci
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/libexec/oci
     * Since hooks under this directory will be used by both runc and docker,
       and neither depends on the other, this package, and others that supply
       plugins, probably need to provide the directory.  I suggest adding
       a "%dir %{_libexecdir}/oci" to the %files section.
[x]: %build honors applicable compiler flags or justifies otherwise.
     * Makefile's invocation of the go compiler adds a build ID and uses
       $LDFLAGS.
[x]: Package contains no bundled libraries without FPC exception.
     * Package does not bundle anything.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: 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.
[ ]: 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.
[ ]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[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 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]: 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]: 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).
[-]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in oci-
     register-machine-debuginfo
[ ]: Package functions as described.
     * Looks straightforward, haven't personally verified it.
[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.
     * Package includes no tests.
[x]: Packages should try to preserve timestamps of original installed
     files.
     * Files being explicitly installed are generated at build-time, and the
       rest are installed by RPM's macros.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Uses parallel make %{?_smp_mflags} macro.
[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:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
     * Actually, it succeeded.
[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.


Installation errors
-------------------
INFO: mock.py version 1.2.14 starting (python version = 3.5.1)...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled ccache
Mock Version: 1.2.14
INFO: Mock Version: 1.2.14
Finish: chroot init
INFO: installing package(s): /misc/oci-machine-register/review-oci-register-machine/results/oci-register-machine-0-0.1.git157f2ef.fc24.x86_64.rpm /misc/oci-machine-register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git157f2ef.fc24.x86_64.rpm /misc/oci-machine-register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git157f2ef.fc24.x86_64.rpm
ERROR: Command failed. See logs for output.
 # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 24 --setopt=deltarpm=false install /misc/oci-machine-register/review-oci-register-machine/results/oci-register-machine-0-0.1.git157f2ef.fc24.x86_64.rpm /misc/oci-machine-register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git157f2ef.fc24.x86_64.rpm /misc/oci-machine-register/review-oci-register-machine/results/oci-register-machine-debuginfo-0-0.1.git157f2ef.fc24.x86_64.rpm --setopt=tsflags=nocontexts


Rpmlint
-------
Checking: oci-register-machine-0-0.1.git157f2ef.fc24.x86_64.rpm
          oci-register-machine-debuginfo-0-0.1.git157f2ef.fc24.x86_64.rpm
          oci-register-machine-0-0.1.git157f2ef.fc24.src.rpm
oci-register-machine.x86_64: W: spelling-error Summary(en_US) Golang -> Golan, Golan g, Angolan
oci-register-machine.x86_64: W: spelling-error Summary(en_US) systemd -> systems, system, system d
oci-register-machine.x86_64: W: spelling-error %description -l en_US Golang -> Golan, Golan g, Angolan
oci-register-machine.x86_64: W: spelling-error %description -l en_US systemd -> systems, system, system d
oci-register-machine.x86_64: W: incoherent-version-in-changelog 0-0.1.git6863 ['0-0.1.git157f2ef.fc24', '0-0.1.git157f2ef']
oci-register-machine.src: W: spelling-error Summary(en_US) Golang -> Golan, Golan g, Angolan
oci-register-machine.src: W: spelling-error Summary(en_US) systemd -> systems, system, system d
oci-register-machine.src: W: spelling-error %description -l en_US Golang -> Golan, Golan g, Angolan
oci-register-machine.src: W: spelling-error %description -l en_US systemd -> systems, system, system d
3 packages and 0 specfiles checked; 0 errors, 9 warnings.




Requires
--------
oci-register-machine (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libpthread.so.0()(64bit)

oci-register-machine-debuginfo (rpmlib, GLIBC filtered):



Provides
--------
oci-register-machine:
    oci-register-machine
    oci-register-machine(x86-64)

oci-register-machine-debuginfo:
    oci-register-machine-debuginfo
    oci-register-machine-debuginfo(x86-64)



Source checksums
----------------
https://github.com/projectatomic/oci-register-machine/archive/157f2efcb85ebe4d45ec8df4d1002e8200ca4d51/oci-register-machine-157f2ef.tar.gz :
  CHECKSUM(SHA256) this package     : b7b1e7fcb05e7d702d1a1042971051463f6c0d91590eb06c859f4a0ee30a6a13
  CHECKSUM(SHA256) upstream package : b7b1e7fcb05e7d702d1a1042971051463f6c0d91590eb06c859f4a0ee30a6a13


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review -n oci-register-machine
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6


Short version: looks good, package approved, setting package-review flag to '+'.
Comment 21 Gwyn Ciesla 2016-02-20 20:39:42 UTC 
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/oci-register-machine
Note You need to log in before you can comment on or make changes to this bug.