Spec URL: https://jdennis.fedorapeople.org/package_review/keycloak-httpd-client-install.spec SRPM URL: https://jdennis.fedorapeople.org/package_review/keycloak-httpd-client-install-0.1-1.fc23.src.rpm Description: Keycloak is a federated Identity Provider (IdP). Apache HTTPD supports a variety of authentication modules which can be configured to utilize a Keycloak IdP to perform authentication. This package contains libraries and tools which can automate and simplify configuring an Apache HTTPD authentication module and registering as a client of a Keycloak IdP. Fedora Account System Username: jdennis
I thought I would address up front what I suspect might be an issue that will be raised during package review. The name of the package is keycloak-httpd-client-install, but the package build produces only 2 python packages, python2-keycloak-httpd-client-install and python3-keycloak-httpd-client-install. One wonders if the package name shouldn't be python-keycloak-httpd-client-install instead of just keycloak-httpd-client-install without the python prefix. Omitting the python prefix on the package name was a conscious and deliberate choice. We expect the keycloak-httpd-client-install package to include more tools down the road, especially components written in other languages (e.g. java). Therefore there is nothing that makes the contents of this package exclusively Python, it just happens that at this moment it only produces two python subpackages just like any other package might also have a python subpackage for it's Python components.
Upstream added a man page. The spec file and SRPM (https://jdennis.fedorapeople.org/package_review/keycloak-httpd-client-install-0.2-1.fc23.src.rpm) were updated to include the man page.
I'm confused about the naming. Why not put the _bindir files into keycloak-httpd-client-install and keep the python parts in subpackages or is it because it would be confusing to install a subpackage to get the top-level package? It would seem better to me to put the common things into keycloak-httpd-client-install and leave the python parts broken out. License does match source, MIT vs GPLv3 rm %{buildroot}/usr/bin/keycloak-httpd-client-install should be rm %{buildroot}%{_bindir}/keycloak-httpd-client-install man pages should not be %doc man page is not installed in python3 subpackage Missing %dir for %{_datadir}/python-%{srcname} Issues with man page (man --warnings keycloak-httpd-client-install.8 >/dev/null) <standard input>:8: warning: macro `Bkeycloak-httpd-client-install' not defined <standard input>:174: warning: macro `BDetermining' not defined Not sure I like the glob for _bindir given there is a single file. Why not just list the one file? Did upstream release 0.2 include only the man page? Wondering if the upgrade to upstream should also be mentioned in changelog.
Thank you Rob for a good review, you caught a number of things I missed and I learned something new (man --warnings) :-) I've updated the spec and srpm in the same location. I addressed all the issues you raised with the exception of the "Missing %dir for %{_datadir}/python-%{srcname}". I believe that is the correct idiom. I want *both* the directory and it's contents packaged, not just the directory (which is what prefixing with %dir would do). A couple of notes: The package now contains a ChangeLog. The package structure was reworked based on your suggestions. There is now a main package (keycloak-httpd-client-install) and it will cause one of or both of the python2-keycloak-httpd-client-install or the python3-keycloak-httpd-client-install sub-packages to be installed depending up which (system) python environment you're installing in. All the common files are in the main package. The Python specific files are in either the python2-keycloak-httpd-client-install or the python3-keycloak-httpd-client-install sub-packages. Only the python3-keycloak-httpd-client-install sub-package installs the keycloak-httpd-client-install script per the packaging guidelines. This is why if you install in a Py2 system environment you'll get all 3 packages, the main package, the py2 package for Py2 tools that might import the package/module, and the Py3 package so the script is present and can run. If all this sounds a bit convoluted it is because the Python packaging guidelines require it.
Updated SRPM after initial review comments: SRPM URL: https://jdennis.fedorapeople.org/package_review/keycloak-httpd-client-install-0.3-1.fc23.src.rpm
Why is %files for the main package separate from the %files for the subpackages? To keep things distinct? [!]: Package must own all directories that it creates. $ ls /usr/share/keycloak-httpd-client-install/ templates $ rpm -q --whatprovides /usr/share/keycloak-httpd-client-install/ file /usr/share/keycloak-httpd-client-install is not owned by any package
===== 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. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [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. [-]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of 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. [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 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]: 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 Python: [x]: Python eggs must not download any dependencies during the build process. [x]: A package which is used by another package via an egg interface should provide egg info. [x]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [x]: Binary eggs must be removed in %prep ===== 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 [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: 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]: 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]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. [x]: Spec file according to URL is the same as in SRPM.
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/keycloak-httpd-client-install
Successfully imported and built for rawhide, f24 and f23. Closing.