[Bug 1301143] New: Review Request: skopeo - Get information about Docker images without pulling them
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Bug ID: 1301143 Summary: Review Request: skopeo - Get information about Docker images without pulling them Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: amurdaca@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://raw.githubusercontent.com/runcom/skopeo/master/skopeo.spec
SRPM URL: http://runcom.ninja/skopeo-0.1.0-1.fc23.src.rpm
Koji builds:
- f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=12650312 - rh: http://koji.fedoraproject.org/koji/taskinfo?taskID=12650093
Description: skopeo is a command line utility which is able to inspect a repository on a Docker registry. It fetches the repository's manifest and it is able to show you a "docker inspect"-like json output about a whole repository or a tag.
Fedora Account System Username: runcom
Additional info: I'm the owner of the package upstream
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Lokesh Mandvekar lsm5@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jchaloup@redhat.com, | |lsm5@redhat.com Assignee|nobody@fedoraproject.org |nalin@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Lokesh Mandvekar lsm5@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR) Flags| |fedora-review?
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #1 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.1-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12661477
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #2 from Antonio Murdaca amurdaca@redhat.com --- I've released a new version, and I've made sure distro-supplied golang libs are used when possible (in spec), the only leftover are (from gofed):
Class: github.com/docker/distribution (golang-github-docker-distribution) PkgDB=False Class: github.com/docker/docker (docker) PkgDB=False Class: github.com/docker/engine-api (golang-github-docker-engine-api) PkgDB=False
---
Spec URL: https://github.com/runcom/skopeo/blob/v0.1.2/skopeo.spec SRPM URL: http://runcom.ninja/skopeo-0.1.2-0.fc23.src.rpm
Koji builds:
- f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=12661579 - rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=12661563
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #3 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.2-0.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12661563
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Lokesh Mandvekar lsm5@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dwalsh@redhat.com
--- Comment #4 from Lokesh Mandvekar lsm5@redhat.com --- Ohh btw, I feel it'd be best first moved to projectatomic github before we go ahead with approving this, the only reason being moving from @runcom to @projectatomic would result in changing golang(github.com/runcom/skopeo) to golang(github.com/projectatomic/skopeo) in the golang paths provided, not too big a deal, but imho something best decided beforehand.
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #5 from Antonio Murdaca amurdaca@redhat.com --- (In reply to Lokesh Mandvekar from comment #4)
Ohh btw, I feel it'd be best first moved to projectatomic github before we go ahead with approving this, the only reason being moving from @runcom to @projectatomic would result in changing golang(github.com/runcom/skopeo) to golang(github.com/projectatomic/skopeo) in the golang paths provided, not too big a deal, but imho something best decided beforehand.
should this binary have a golang(...)-like name resolvable via golang(...) macro? I've read golang binaries can be named without golang-github-* prefix so this won't suffer from renaming (also this do not provide a devel package) but just let me know and I'll move the repository as fast as I can and fix what's needed
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #6 from Jan Chaloupka jchaloup@redhat.com --- Wondering if it would be better to create github repository (e.g. fedora/golang-reviews) and instead of posting links to spec to post links to pull request and comment in the PR. Missing feature to comment lines of the spec file.
Well, the devel subpackage is usefull for analysis. When the devel subpackage is present, it can be scanned for dependencies and other info about the code. At the moment we are running simple scans of new builds of golang projects in Koji. In future, we plan to report missing or broken dependencies. So if you provide the devel subpackage, you get automatic scans and reports about health of your package.
Second, I would recommend to add some macro at the top of the spec. E.g. commit, provider_prefix, import_path. They are used for automatic updates of spec file (e.g. 'gofed bump') and in analysis (as described above).
Third, you are using 'go build' inside Makefile. So your project can be built on architectures with golang compiler only. If you move the commands into %build section, you can use %gobuild macro a gain support for debugingo and architectures with gcc-go compiler.
You can play with gofed for a while, try to run: # yum install gofed $ gofed repo2spec --detect github.com/runcom/skopeo --with-build --with-extra
Bad news is it depends on docker so the devel subpackage will not be complete. The good new is you can build the project from bundled dependencies.
Lokesh, we should definitely do something about the docker. This is another project that depends on it. If this is happening we should at least partially built the package from bundled and partially from debundled deps. At this point this is out of the question as we are still missing automatic tools that would do all the hard work for us.
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #7 from Jan Chaloupka jchaloup@redhat.com --- btw. a lot of new dependencies :)
$ gofed repo2spec --detect github.com/runcom/skopeo --with-build --with-extra Repo URL: github.com/runcom/skopeo Commit: 572a6b6f537d71f7cabfdcfe185c6d7cb4367272 Name: golang-github-runcom-skopeo
(1/4) Checking if the package already exists in PkgDB (2/4) Downloading tarball (3/4) Generating spec file (4/4) Discovering golang dependencies Class: github.com/Azure/go-ansiterm (golang-github-Azure-go-ansiterm) PkgDB=False Class: github.com/Sirupsen/logrus (golang-github-Sirupsen-logrus) PkgDB=True Class: github.com/codegangsta/cli (golang-github-codegangsta-cli) PkgDB=True Class: github.com/docker/distribution (golang-github-docker-distribution) PkgDB=False Class: github.com/docker/docker (docker) PkgDB=False Class: github.com/docker/engine-api (golang-github-docker-engine-api) PkgDB=False Class: github.com/docker/go-connections (golang-github-docker-go-connections) PkgDB=False Class: github.com/docker/go-units (golang-github-docker-go-units) PkgDB=False Class: github.com/docker/libtrust (golang-github-docker-libtrust) PkgDB=True Class: github.com/go-check/check (golang-github-go-check-check) PkgDB=False Class: github.com/gorilla/context (golang-github-gorilla-context) PkgDB=True Class: github.com/gorilla/mux (golang-github-gorilla-mux) PkgDB=True Class: github.com/opencontainers/runc (golang-github-opencontainers-runc) PkgDB=False Class: github.com/vbatts/tar-split (golang-github-vbatts-tar-split) PkgDB=False Class: golang.org/x/net (golang-googlecode-net) PkgDB=True
Spec file golang-github-runcom-skopeo.spec at /home/jchaloup/Packages/reviews/skopeo/golang-github-runcom-skopeo/fedora/golang-github-runcom-skopeo
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #8 from Antonio Murdaca amurdaca@redhat.com --- (In reply to Jan Chaloupka from comment #7)
btw. a lot of new dependencies :)
$ gofed repo2spec --detect github.com/runcom/skopeo --with-build --with-extra Repo URL: github.com/runcom/skopeo Commit: 572a6b6f537d71f7cabfdcfe185c6d7cb4367272 Name: golang-github-runcom-skopeo
(1/4) Checking if the package already exists in PkgDB (2/4) Downloading tarball (3/4) Generating spec file (4/4) Discovering golang dependencies Class: github.com/Azure/go-ansiterm (golang-github-Azure-go-ansiterm) PkgDB=False Class: github.com/Sirupsen/logrus (golang-github-Sirupsen-logrus) PkgDB=True Class: github.com/codegangsta/cli (golang-github-codegangsta-cli) PkgDB=True Class: github.com/docker/distribution (golang-github-docker-distribution) PkgDB=False Class: github.com/docker/docker (docker) PkgDB=False Class: github.com/docker/engine-api (golang-github-docker-engine-api) PkgDB=False Class: github.com/docker/go-connections (golang-github-docker-go-connections) PkgDB=False Class: github.com/docker/go-units (golang-github-docker-go-units) PkgDB=False Class: github.com/docker/libtrust (golang-github-docker-libtrust) PkgDB=True Class: github.com/go-check/check (golang-github-go-check-check) PkgDB=False Class: github.com/gorilla/context (golang-github-gorilla-context) PkgDB=True Class: github.com/gorilla/mux (golang-github-gorilla-mux) PkgDB=True Class: github.com/opencontainers/runc (golang-github-opencontainers-runc) PkgDB=False Class: github.com/vbatts/tar-split (golang-github-vbatts-tar-split) PkgDB=False Class: golang.org/x/net (golang-googlecode-net) PkgDB=True
Spec file golang-github-runcom-skopeo.spec at /home/jchaloup/Packages/reviews/skopeo/golang-github-runcom-skopeo/fedora/ golang-github-runcom-skopeo
Yes, those new dependencies come from the master branch, I can tag a new release and add all those dependencies :)
(In reply to Jan Chaloupka from comment #6)
Wondering if it would be better to create github repository (e.g. fedora/golang-reviews) and instead of posting links to spec to post links to pull request and comment in the PR. Missing feature to comment lines of the spec file.
Well, the devel subpackage is usefull for analysis. When the devel subpackage is present, it can be scanned for dependencies and other info about the code. At the moment we are running simple scans of new builds of golang projects in Koji. In future, we plan to report missing or broken dependencies. So if you provide the devel subpackage, you get automatic scans and reports about health of your package.
Second, I would recommend to add some macro at the top of the spec. E.g. commit, provider_prefix, import_path. They are used for automatic updates of spec file (e.g. 'gofed bump') and in analysis (as described above).
Third, you are using 'go build' inside Makefile. So your project can be built on architectures with golang compiler only. If you move the commands into %build section, you can use %gobuild macro a gain support for debugingo and architectures with gcc-go compiler.
You can play with gofed for a while, try to run: # yum install gofed $ gofed repo2spec --detect github.com/runcom/skopeo --with-build --with-extra
Bad news is it depends on docker so the devel subpackage will not be complete. The good new is you can build the project from bundled dependencies.
Lokesh, we should definitely do something about the docker. This is another project that depends on it.
If this is happening we should at least partially built the package from bundled and partially from debundled deps.
This is what I'm actually doing - I'm removing each bundled deps with rm -rf /vendor and just leave under vendor the ones from docker which cannot be debundled :)
At this point this is out of the question as we are still missing automatic tools that would do all the hard work for us.
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #9 from Jan Chaloupka jchaloup@redhat.com --- Some of them are already packaged, just with a different name.
should this binary have a golang(...)-like name resolvable via golang(...) macro?
No. golang(...) virtual provides is used only for dependencies
I've read golang binaries can be named without golang-github-* prefix so this won't suffer from renaming
Yeap. It can be named anyway you wish. All projects packaged in fedora with intent to provide at least one binary are named by repository. The standard way is to use project name.
(also this do not provide a devel package)
As I mentioned above. At least you can provide devel package with no [Build]Requires for sake of analysis. The generated spec file already provides all code necessary. Just with some small modifications.
but just let me know and I'll move the repository as fast as I can and fix what's needed
If you use generated macros, it is two-liner fix
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #10 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.3-0.1.git572a6b6.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12713132
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #11 from Antonio Murdaca amurdaca@redhat.com --- Jan, Lokesh, I should have fixed everything now.
The only thing which doesn't work is removing the `cd $(pwd)/_build/...)` stuff in %install (as Jan suggested me to do) because I believe it is tied to $GO15VENDOREXPERIMENT and "go build" only works with "vendor/" if the current source being built lives under $GOPATH. (you can try this and see it can't resolves package dependencies)
I've also tagged a new release (v0.1.3)
Spec URL: https://github.com/runcom/skopeo/blob/master/skopeo.spec SRPM URL: http://runcom.ninja/skopeo-0.1.3-0.1.gitfdb5cac.fc23.src.rpm
Koji builds:
- f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=12713377 - rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=12713364
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #12 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.3-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12716264
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #13 from Antonio Murdaca amurdaca@redhat.com --- Pulled in the latest Jan's fixes to the spec (thanks a lot)
Spec URL: https://raw.githubusercontent.com/runcom/fedora-pkgs/master/skopeo/fedora/sk... SRPM URL: https://github.com/runcom/fedora-pkgs/raw/master/skopeo/skopeo-0.1.3-1.fc23....
Koji builds:
- f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=12716275 - rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=12716276
Thanks for the help guys!
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #14 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.3-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12716275
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #15 from Nalin Dahyabhai nalin@redhat.com --- runcom is not currently in the packagers group; I can sponsor.
I've got questions about the license tag when we're bundling, and could probably use some clarification about whether or not, and if so, how many, of the vendored modules need to be debundled for Fedora. Otherwise it looks pretty straightforward from here.
fedora-review output:
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - Sources used to build the package match the upstream source, as provided in the spec URL. See: http://fedoraproject.org/wiki/Packaging/SourceURL
Since you're keeping a .spec file in the repository, I expect you'll be keeping it more or less in sync with the one being used for Fedora, so no worries there.
The source tarball in the SRPM contained the .git directory and copies of generated files, including the binary, which is rather odd. How was it generated? Will future versions of the package do this as well?
- Package uses either %{buildroot} or $RPM_BUILD_ROOT Note: Using both %{buildroot} and $RPM_BUILD_ROOT See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros
Stylistically, it's better to choose either %{buildroot} or $RPM_BUILD_ROOT and be consistent about it. You're not doing anything that makes either of them not an option, so use whichever you prefer.
- 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. Note: License file LICENSE is marked as %doc instead of %license See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
The main package tags the LICENSE file as %doc rather than %license, which is trivially fixable.
===== 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.
I suspect that linking your MIT-licensed main logic with vendored sources from other repositories is going to produce
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Mixed_Source_Li...
[ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "MIT/X11 (BSD like)", "BSD (3 clause)", "*No copyright* Apache (v2.0)", "Unknown or generated", "BSD (2 clause)". 371 files have unknown license. Detailed output of licensecheck in /misc/skopeo /review-skopeo/licensecheck.txt
If it's not mixed source, then "License: MIT" is correct.
[x]: %build honors applicable compiler flags or justifies otherwise. Uses %gobuild to invoke the go compiler. [ ]: Package contains no bundled libraries without FPC exception.
Package bundles several libraries. Does it need to remove them at the end of the %setup section when %{with_bundled} is 0 in order to ensure that the compiler picks up the debundled copies? Doesn't Fedora require debundling?
[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. [!]: Package consistently uses macros (instead of hard-coded directory names).
The package's makefile hardcodes the install locations, and we don't force them to match %{_bindir} and %{_mandir}.
[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. [ ]: Spec file is legible and written in American English.
The package could use a longer %description; I think something along the lines of the paragraph that starts around line 6 of README.md would work.
[-]: Package contains systemd file(s) if in need. [ ]: Useful -debuginfo package or justification otherwise.
Why is %{with_debug} disabled? Rebuilding it with debuginfo enabled produces files with names that seem to be of some use to my debugger.
[-]: 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 2 files. [ ]: Package complies to the Packaging Guidelines
Aside from questions I have about licensing and bundling, this looks fine.
[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]: 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]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [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]: 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. [-]: 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.
The makefile doesn't bother for generated files, but then they're 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]: 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: skopeo-0.1.3-0.1.gitfdb5cac.fc24.x86_64.rpm skopeo-0.1.3-0.1.gitfdb5cac.fc24.src.rpm skopeo.x86_64: W: unstripped-binary-or-object /usr/bin/skopeo skopeo.src: W: file-size-mismatch skopeo-fdb5cac.tar.gz = 5646750, https://github.com/runcom/skopeo/archive/fdb5cac7f5af50ae5b1e3424965c31600d8... = 428362 2 packages and 0 specfiles checked; 0 errors, 2 warnings.
Rpmlint (installed packages) ---------------------------- sh: /usr/bin/python: No such file or directory skopeo.x86_64: W: unstripped-binary-or-object /usr/bin/skopeo 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Requires -------- skopeo (rpmlib, GLIBC filtered): libc.so.6()(64bit) libpthread.so.0()(64bit) rtld(GNU_HASH)
Provides -------- skopeo: skopeo skopeo(x86-64)
Source checksums ---------------- https://github.com/runcom/skopeo/archive/fdb5cac7f5af50ae5b1e3424965c31600d8... : CHECKSUM(SHA256) this package : 1d2050f0edfec3abb3b7cff2d8d71def51b7057875ee6a82e485d7e1eb9fd196 CHECKSUM(SHA256) upstream package : bae74476f07f2955ed360c78ce7f724896a4479e4b6628a37ce34a08e605cb63 diff -r also reports differences
Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20 Command line :/usr/bin/fedora-review -n skopeo 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
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Nalin Dahyabhai nalin@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Blocks|177841 (FE-NEEDSPONSOR) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #16 from Antonio Murdaca amurdaca@redhat.com --- (In reply to Nalin Dahyabhai from comment #15)
runcom is not currently in the packagers group; I can sponsor.
I've got questions about the license tag when we're bundling, and could probably use some clarification about whether or not, and if so, how many, of the vendored modules need to be debundled for Fedora. Otherwise it looks pretty straightforward from here.
fedora-review output:
Package Review
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues:
Sources used to build the package match the upstream source, as provided in the spec URL. See: http://fedoraproject.org/wiki/Packaging/SourceURL
Since you're keeping a .spec file in the repository, I expect you'll be
keeping it more or less in sync with the one being used for Fedora, so no worries there.
I've created https://github.com/runcom/fedora-pkgs where I do have .spec(s) and SRPM(s) I'm planning to remove the .spec from the source repo and maintain the .spec at https://github.com/runcom/fedora-pkgs/tree/master/skopeo/fedora/skopeo
The source tarball in the SRPM contained the .git directory and copies of generated files, including the binary, which is rather odd. How was it generated? Will future versions of the package do this as well?
Probably a mistake - I've regenerated the SRPM and it's available at https://github.com/runcom/fedora-pkgs/raw/master/skopeo/fedora/skopeo/skopeo... It does not contain .git nor binaries anymore.
Package uses either %{buildroot} or $RPM_BUILD_ROOT Note: Using both %{buildroot} and $RPM_BUILD_ROOT See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros
Stylistically, it's better to choose either %{buildroot} or
$RPM_BUILD_ROOT and be consistent about it. You're not doing anything that makes either of them not an option, so use whichever you prefer.
Fixed in https://raw.githubusercontent.com/runcom/fedora-pkgs/master/skopeo/fedora/sk...
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. Note: License file LICENSE is marked as %doc instead of %license See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
The main package tags the LICENSE file as %doc rather than %license, which
is trivially fixable.
Fixed in https://raw.githubusercontent.com/runcom/fedora-pkgs/master/skopeo/fedora/sk...
===== 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.
I suspect that linking your MIT-licensed main logic with vendoredsources from other repositories is going to produce
https://fedoraproject.org/wiki/Packaging: LicensingGuidelines#Mixed_Source_Licensing_Scenario
[ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "MIT/X11 (BSD like)", "BSD (3 clause)", "*No copyright* Apache (v2.0)", "Unknown or generated", "BSD (2 clause)". 371 files have unknown license. Detailed output of licensecheck in /misc/skopeo /review-skopeo/licensecheck.txt
If it's not mixed source, then "License: MIT" is correct.
This license stuff is tricky...I've re-licensed my tool with ASL 2.0 since all my vendors are ASL 2.0 afaict.
[x]: %build honors applicable compiler flags or justifies otherwise. Uses %gobuild to invoke the go compiler. [ ]: Package contains no bundled libraries without FPC exception.
Package bundles several libraries. Does it need to remove them at theend of the %setup section when %{with_bundled} is 0 in order to ensure that the compiler picks up the debundled copies? Doesn't Fedora require debundling?
I've added the removal of vendor/ when %{with_bundled} is 0 Right now, unluckily, this tool won't build from debundled vendor's copies, and also not all vendors are already in Fedora - but I'm working on packaging them with Lokesh and Jan for a future version
[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. [!]: Package consistently uses macros (instead of hard-coded directory names).
The package's makefile hardcodes the install locations, and we don'tforce them to match %{_bindir} and %{_mandir}.
Fixed
[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. [ ]: Spec file is legible and written in American English.
The package could use a longer %description; I think something alongthe lines of the paragraph that starts around line 6 of README.md would work.
Fixed in https://raw.githubusercontent.com/runcom/fedora-pkgs/master/skopeo/fedora/sk...
[-]: Package contains systemd file(s) if in need. [ ]: Useful -debuginfo package or justification otherwise.
Why is %{with_debug} disabled? Rebuilding it with debuginfo enabledproduces files with names that seem to be of some use to my debugger.
Enabled %{with_debug} - was an old .spec
[-]: 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 2 files. [ ]: Package complies to the Packaging Guidelines
Aside from questions I have about licensing and bundling, this looksfine.
[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]: 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]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [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]: 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. [-]: 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.
The makefile doesn't bother for generated files, but then they'regenerated at build-time.
should I call `make clean` to remove generated files in a %clean section?
[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: [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: skopeo-0.1.3-0.1.gitfdb5cac.fc24.x86_64.rpm skopeo-0.1.3-0.1.gitfdb5cac.fc24.src.rpm skopeo.x86_64: W: unstripped-binary-or-object /usr/bin/skopeo skopeo.src: W: file-size-mismatch skopeo-fdb5cac.tar.gz = 5646750, https://github.com/runcom/skopeo/archive/ fdb5cac7f5af50ae5b1e3424965c31600d86232c/skopeo-fdb5cac.tar.gz = 428362 2 packages and 0 specfiles checked; 0 errors, 2 warnings.
Rpmlint (installed packages)
sh: /usr/bin/python: No such file or directory skopeo.x86_64: W: unstripped-binary-or-object /usr/bin/skopeo 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
Requires
skopeo (rpmlib, GLIBC filtered): libc.so.6()(64bit) libpthread.so.0()(64bit) rtld(GNU_HASH)
Provides
skopeo: skopeo skopeo(x86-64)
Source checksums
https://github.com/runcom/skopeo/archive/ fdb5cac7f5af50ae5b1e3424965c31600d86232c/skopeo-fdb5cac.tar.gz : CHECKSUM(SHA256) this package : 1d2050f0edfec3abb3b7cff2d8d71def51b7057875ee6a82e485d7e1eb9fd196 CHECKSUM(SHA256) upstream package : bae74476f07f2955ed360c78ce7f724896a4479e4b6628a37ce34a08e605cb63 diff -r also reports differences
Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20 Command line :/usr/bin/fedora-review -n skopeo 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
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #17 from Antonio Murdaca amurdaca@redhat.com --- New stuff (v0.1.4) after Nalin's review:
Spec URL: https://raw.githubusercontent.com/runcom/fedora-pkgs/master/skopeo/fedora/sk... SRPM URL: https://github.com/runcom/fedora-pkgs/raw/master/skopeo/fedora/skopeo/skopeo...
Koji builds:
- f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=12722010 - rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=12722004
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #18 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.4-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12722010
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Jan Chaloupka jchaloup@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |amurdaca@redhat.com Flags| |needinfo?(amurdaca@redhat.c | |om)
--- Comment #19 from Jan Chaloupka jchaloup@redhat.com --- After reading [1] I can not figure out why is GO15VENDOREXPERIMENT env needed. Can you help me to understand?
[1] https://docs.google.com/document/d/1Bz5-UB7g2uPBdOx-rw5t9MxJwkfpx90cqG9AFL0J...
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Antonio Murdaca amurdaca@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(amurdaca@redhat.c | |om) |
--- Comment #20 from Antonio Murdaca amurdaca@redhat.com --- (In reply to Jan Chaloupka from comment #19)
After reading [1] I can not figure out why is GO15VENDOREXPERIMENT env needed. Can you help me to understand?
[1] https://docs.google.com/document/d/1Bz5-UB7g2uPBdOx- rw5t9MxJwkfpx90cqG9AFL0JAYo/edit
Quoting from the doc:
This change will only be enabled if the go command is run with GO15VENDOREXPERIMENT=1 in its environment.
So, the behavior of looking into $pwd/vendor/... instead of resolving deps from $GOPATH is only enabled if you pass that env variable - otherwise it uses $GOPATH (and maybe $GOROOT). This is in go1.5, eventually we will have this as the default behavior in go1.6, from the doc:
If we decide that the vendor behavior is correct, then in a later release (possibly Go 1.6) we would make the vendor behavior default on. Projects containing “vendor” directories could still use “GO15VENDOREXPERIMENT=0” to get the old behavior while they convert their code. In a still later release (possibly Go 1.7) we would remove the use of the environment variable, locking in the vendoring semantics.
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #21 from Nalin Dahyabhai nalin@redhat.com --- (In reply to Antonio Murdaca from comment #16)
I've created https://github.com/runcom/fedora-pkgs where I do have .spec(s) and SRPM(s) I'm planning to remove the .spec from the source repo and maintain the .spec at https://github.com/runcom/fedora-pkgs/tree/master/skopeo/fedora/skopeo
FWIW, Fedora's packaging repository will keep its own copy in a branch for each Fedora release (see repositories at http://pkgs.fedoraproject.org/cgit/rpms/ for examples).
This license stuff is tricky...I've re-licensed my tool with ASL 2.0 since all my vendors are ASL 2.0 afaict.
Yeah, that should be accurate either way.
I've added the removal of vendor/ when %{with_bundled} is 0 Right now, unluckily, this tool won't build from debundled vendor's copies, and also not all vendors are already in Fedora - but I'm working on packaging them with Lokesh and Jan for a future version
Understood, it's a work in progress.
should I call `make clean` to remove generated files in a %clean section?
Historically, %clean's job was to clean up $RPM_BUILD_ROOT, way back when even using a buildroot was optional. Now that the buildroot is set and cleaned up by default, Fedora doesn't expect you to have a %clean section.
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #22 from Nalin Dahyabhai nalin@redhat.com --- Looks good, though the -devel package contains sources in package "main". My guess is that you wanted to leave with_devel set to 0 all the time.
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Nalin Dahyabhai nalin@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #23 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- runcom's scratch build of skopeo-0.1.4-1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12786664
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #24 from Antonio Murdaca amurdaca@redhat.com --- Updated everything and it should be good to go:
Spec URL: https://raw.githubusercontent.com/runcom/fedora-pkgs/master/skopeo/fedora/sk... SRPM URL: https://github.com/runcom/fedora-pkgs/raw/master/skopeo/fedora/skopeo/skopeo...
Koji builds:
- f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=12786664 - rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=12786653
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #25 from Nalin Dahyabhai nalin@redhat.com --- Yup, looks good from here. Thanks!
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Nalin Dahyabhai nalin@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Jason Tibbitts tibbs@math.uh.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 (FE-NEEDSPONSOR) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
--- Comment #26 from Jon Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/skopeo
https://bugzilla.redhat.com/show_bug.cgi?id=1301143
Lokesh Mandvekar lsm5@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |RAWHIDE Last Closed| |2016-04-02 00:35:50
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla