Spec URL: https://github.com/cgwalters/min-metadata-service/blob/master/packaging/min-metadata-service.spec.in SRPM URL: https://github.com/cgwalters/min-metadata-service (Ok, not a srpm, but you can generate one with: make -f Makefile.dist-packaging srpm ) Description: Minimal client implementation of AWS/OpenStack metadata API Fedora Account System Username: walters
FTR, it makes reviewing easier if you actually provide the spec and source RPM, because fedora-review is expecting them. But I'll take a look anyway. :)
I ran into a basic showstopper because this depends on pkgconfig(libgsystem), and it looks like libgsystem is Not Yet A Thing. Other things: Since you don't have an upstream source URL, you should precede that line with a comment explaining how to generate the tar.xz. See https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL The %clean section and rm -rf $RPM_BUILD_ROOT aren't needed unless this is going into EPEL 5. For Fedora and newer epel, these are unneeded. Ditto %defattr. There is no %changelog. We need one. Need require systemd, because of the .service file. (More specifically, for the ownership of the directory that file goes in.) Very minor: I'd like to see the Summary be a little more descriptive. And, currently, doesn't seem to mention OpenStack in either the summary or description. Also: your COPYING File has the wrong address for the FSF.
(In reply to Matthew Miller from comment #2) > Since you don't have an upstream source URL, you should precede that line > with a comment explaining how to generate the tar.xz. See > https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL Actually, as can be read on the referred page, it's easily possible to provide a valid source URL as Github offers automated archiving of both tags and revisions: https://fedoraproject.org/wiki/Packaging:SourceURL#Github Using this source URL (and the tarball that comes from it) is clearly preferred over using your self-created tarball unless there's good reasons against it (which you also should add to the comment preceding the source non-URL).
(In reply to Sandro Mathys from comment #3) > Using this source URL (and the tarball that comes from it) is clearly > preferred over using your self-created tarball unless there's good reasons > against it (which you also should add to the comment preceding the source > non-URL). I have generic infrastructure (in the Makefile.dist-packaging) for generating tarballs via "git archive" that I use for most of my projects. The approach above only works for Github - not all of my projects are there.
(In reply to Matthew Miller from comment #2) > I ran into a basic showstopper because this depends on > pkgconfig(libgsystem), and it looks like libgsystem is Not Yet A Thing. It is now A Thing =) http://koji.fedoraproject.org/koji/buildinfo?buildID=499398 > There is no %changelog. We need one. I don't have %changelog in the spec files I maintain inside the modules, since git operates as a perfectly fine changelog. When the spec file gets forked for inclusion in Fedora, I'll add one per the current requirements. Fixed everything else, thanks! https://github.com/cgwalters/min-metadata-service/commit/410d7024fdcdcdbb5d31c709b3dd389b0ea68e43 https://github.com/cgwalters/min-metadata-service/commit/dd344bd58a27e6875badc92fea264d245594ae2b
I hate to be picky, but the command in the spec comment should have the tag or commit that matches the version in the tarball. The goal is that someone should be able to create the exact same thing independently for verification, not just as a convenience for updating. And I also hate to be difficult about the changelog, but that's a "must" item in the packaging guidelines. Can you make the forked package for inclusion in Fedora at this point? That's what's supposed to be being reviewed, in any case. Those points aside, the actual thing looks good, so I'll pass the review as soon as it's available.
(In reply to Matthew Miller from comment #6) > I hate to be picky, but the command in the spec comment should have the tag > or commit that matches the version in the tarball. The goal is that someone > should be able to create the exact same thing independently for > verification, not just as a convenience for updating. Being picky is good! Particularly so I can show you how I do things, and you'll be better able to understand the model when I get around to some proposals for changes on the Fedora/RPM source side. So the source tarball name with "Makefile.dist-packaging" is always named after a variant of "git describe". In this case, the tarball is being built from a tag named "v2014.1", so the version is just 2014.1. If I make a tarball from a later commit, it looks like this: ostree-2014.1.36.gb762c2f.tar.xz That means we're 36 commits after the tag 2014.1, and the commit is "b762c2f". > And I also hate to be difficult about the changelog, but that's a "must" > item in the packaging guidelines. For now... =) > Can you make the forked package for > inclusion in Fedora at this point? That's what's supposed to be being > reviewed, in any case. Sure.
(In reply to Colin Walters from comment #7) > So the source tarball name with "Makefile.dist-packaging" is always named > after a variant of "git describe". In this case, the tarball is being built > from a tag named "v2014.1", so the version is just 2014.1. That seems reasonable, but the point of the comment is so the person looking doesn't have to figure out of you are reasonable or not, and in which of several perfectly valid ways *you* are reasonable. :) So it should go into the specfile comment. >> And I also hate to be difficult about the changelog, but that's a "must" >> item in the packaging guidelines. > For now... =) A lot of *all* of this is about a separation between developer and packager roles. You're acting as both, and I think have a bias towards the developer side. That's not inherently bad, but tilting the way we do things in that direction is... well, too big of a conversation for this bug. :)
SPEC: http://fedorapeople.org/~walters/min-metadata-service.spec SRPM: http://fedorapeople.org/~walters/min-metadata-service-2014.3-1.el7.src.rpm
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]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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: "LGPL", "Unknown or generated". 2 files have unknown license. Detailed output of licensecheck in /home/mattdm/tmp/1066176-min-metadata- service/licensecheck.txt [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. [-]: 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. [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 30720 bytes in 1 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [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 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]: 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 do not use a name that already exist [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: SourceX tarball generation or download is documented. Note: Package contains tarball without URL, check comments [ ]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [ ]: Package should compile and build into binary rpms on all supported architectures. [ ]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [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]: Dist tag is present (not strictly required in GL). [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]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [!]: Spec file according to URL is the same as in SRPM. Note: Spec file as given by url is not the same as in SRPM (see attached diff). See: (this test has no URL) [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 Rpmlint ------- Checking: min-metadata-service-2014.3-1.fc21.x86_64.rpm min-metadata-service-2014.3-1.fc21.src.rpm min-metadata-service.x86_64: W: only-non-binary-in-usr-lib min-metadata-service.src:46: E: hardcoded-library-path in %{_prefix}/lib/systemd/system/min-metadata.service min-metadata-service.src:8: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 8) min-metadata-service.src: E: specfile-error warning: bogus date in %changelog: Sun Mar 01 2014 Colin Walters <walters> - 2014.3-1 2 packages and 0 specfiles checked; 2 errors, 2 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint min-metadata-service min-metadata-service.x86_64: W: only-non-binary-in-usr-lib 1 packages and 0 specfiles checked; 0 errors, 1 warnings. # echo 'rpmlint-done:' Diff spec file in url and in SRPM --------------------------------- --- /home/mattdm/tmp/1066176-min-metadata-service/srpm/min-metadata-service.spec2014-03-03 11:56:50.321049172 -0500 +++ /home/mattdm/tmp/1066176-min-metadata-service/srpm-unpacked/min-metadata-service.spec 2014-03-02 16:03:11.000000000 -0500 @@ -47,5 +47,5 @@ %changelog -* Sun Mar 02 2014 Colin Walters <walters> - 2014.3-1 +* Sun Mar 01 2014 Colin Walters <walters> - 2014.3-1 - Initial packaging Requires -------- min-metadata-service (rpmlib, GLIBC filtered): /bin/sh libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3.1)(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libgsystem.so.0()(64bit) libpthread.so.0()(64bit) libsoup-2.4.so.1()(64bit) rtld(GNU_HASH) systemd systemd-units Provides -------- min-metadata-service: min-metadata-service min-metadata-service(x86-64) Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13 Command line :/usr/bin/fedora-review -b 1066176 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
Review passed. But fix the date in the changelog (should be Sun Mar 02 2014 or Sat Mar 01, whichever you meant) or else rpmlint will complain about it forever after. :)
New Package SCM Request ======================= Package Name: min-metadata-service Short Description: Minimal client implementation of EC2/OpenStack metadata API Owners: walters mattdm Branches: rawhide InitialCC: walters
InitialCC needs a FAs account, not email, and don't request rawhide, devel is automatic.
New Package SCM Request ======================= Package Name: min-metadata-service Short Description: Minimal client implementation of EC2/OpenStack metadata API Owners: walters mattdm Branches: InitialCC:
Git done (by process-git-requests).