Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.1.1-2.fc18.src.rpm Description: Provides a kerberos authentication service based plugin for OpenShift Fedora Account System Username: tdawson maxamillion
rpmlint output: $ rpmlint rubygem-openshift-origin-auth-kerberos.spec /home/quake/rpmbuild/SRPMS/rubygem-openshift-origin-auth-kerberos-1.1.1-2.fc18.src.rpm /home/quake/rpmbuild/RPMS/noarch/rubygem-openshift-origin-auth-kerberos-1.1.1-2.fc18.noarch.rpm /home/quake/rpmbuild/RPMS/noarch/rubygem-openshift-origin-auth-kerberos-doc-1.1.1-2.fc18.noarch.rpm rubygem-openshift-origin-auth-kerberos.noarch: W: non-conffile-in-etc /etc/openshift/plugins.d/openshift-origin-auth-kerberos.conf.example 3 packages and 1 specfiles checked; 0 errors, 1 warnings. $ /etc/openshift/plugins.d/openshift-origin-auth-kerberos.conf.example is an example and shouldn't be labeled as a config file. So this rpmlint looks correct.
Hi, a few comments : - there is a empty changelog - wouldn't the example file be better in the documentation ? - why does this package requires selinux-policy-targeted and policycoreutils-python ? - while i prefer to have separate spec for EL6 and Fedora, I think would reuse the macro %scl for BuildRequires: ruby193-build if i would have done the same trick with scl. - wouldn't /etc/openshift/plugins.d/ be unowned ? ( no rawhide to check )
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.1.1-3.fc18.src.rpm - fixed changelog - moved example file to documentation - removed requires selinux-policy-targeted and policycoreutils-python -- added requires rubygem(openshift-origin-controller), which is required, but not pulled in with rubygem(openshift-origin-common) - owns /etc/openshift/plugins.d/ -- Checked on rawhide, and we have several plugins that use that directory, and none of them currently own it. I will work on getting them to own the directory. Since we don't know how many, or if any of these plugins will be installed, I'll have each own it. - Left BuildRequires: ruby193-build as it is -- This is a SCL only package, just like scl-utils-build. Using the {scl_prefix} stuff doesn't work for it.
I mean writing BuildRequires: %scl-build instead of BuildRequires: ruby193-build ( maybe I am taking DRY principle too seriously ) For the plugin and missing directory, why not place the directory in rubygem(openshift-origin-common) or a common rpm ? ( I still didn't looked closely on all the rpms on rawhide, but usually, having it placed in 1 single rpm required by the package is easier and prevent errors when someone forget to include the directory )
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.1.1-4.fc18.src.rpm - removed owning /etc/openshift/plugins.d/ -- Will work on getting it owned by -common - I now see what you mean about ruby193-build amd %scl-build -- I don't feel comfortable doing that currently because I know upstream tested alot to get these spec files to build on the Jenkin's instances. Changing it to %scl-build might break things. -- It should be easy enough to test upstream, if I can get any of my code pushed through. If it does work, I'll work on getting it into upstreams spec files.
http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.1.1-4.fc18.src.rpm => 404
Sorry about that. The files are there now.
Hi Troy, Not sure what was your intention, but the firs four lines: %if 0%{?fedora}%{?rhel} <= 6 %global scl ruby193 %global scl_prefix ruby193- %endif they should not be in the spec in Fedora, EPEL nor for RHEL. Please remove them from the spec file. This is not how SCL was intended to work and moreover, it is against fedora's and EPEL's guidelines. Actually the same applies for the following lines: %if 0%{?fedora}%{?rhel} <= 6 BuildRequires: ruby193-build BuildRequires: scl-utils-build %endif
Hi Vit, How is the SCL intended to work? This is how the upstream vendor (OpenShift) got it to work for them. As far as I know, OpenShift is the only project activily using SCL. But if there is another active project successfully using SCL, please point me to them and I can work with OpenShift so they implement SCL in the approved manner. I would like to keep out spec as close as possible with the upstream vendor. If I were to remove all the scl stuff, it would affect much more than the two sections that you pointed out.
Hey Troy, the specfile really seems to be strange, to say the least. Comments for the first condition that Vit has mentioned: - %scl shouldn't actually be defined in the specfile, it should be left to ruby193-build to be present in minimal buildroot. - Defining %scl_prefix is wrong. If ruby193-build is in minimal buildroot, it will draw in scl-utils-build and %scl_prefix will be defined. You really shouldn't hack these things like that. The another condition: - Please don't do that. Again, that is supposed to be solved by those packages being/not being present in the minimal buildroot. The general idea of SCLs is that you can build the same SRPM and get SCL or non-SCL RPMs, based on the buildroot you use. By hardcoding the rhel conditionals, you break this. Some more comments: - %{?scl:%scl_prefix} in Requires and BuildRequires can be shortened to %{?scl_prefix} - The two Requires without %{?scl:%scl_prefix} seem to be suspicious. I'd say that if you build this for ruby193 SCL, this gem won't be able to use them, as they won't be in GEM_PATH (because they're not SCL).
Hi, I will pull out all the SCL stuff so this can make it into Fedora. I'd like to talk with you offline so we can work with OpenShift upstream on their rubygem spec files. They need to be able to have the packages be built by tito, as well as Koji/Brew. What you see is what they found worked for them. If there is a better, more prefered way, I'm sure they would like to hear it. If we can get the spec file to work with upstream, and still be acceptable with Fedora standards, that would be great. If not, hopefully we can get upstreams spec to not be so different that it's painful to convert to Fedora standards. Expect a new spec/srpm soon.
(In reply to comment #11) > Hi, > I will pull out all the SCL stuff so this can make it into Fedora. You don't have to remove everything, just remove the lines we referenced, the rest should be OK, since scl macros are not prohibited in Fedora. > I'd like to talk with you offline so we can work with OpenShift upstream on > their rubygem spec files. Definitely. > They need to be able to have the packages be > built by tito, as well as Koji/Brew. What you see is what they found worked > for them. If there is a better, more prefered way, I'm sure they would like > to hear it. > > If we can get the spec file to work with upstream, and still be acceptable > with Fedora standards, that would be great. If not, hopefully we can get > upstreams spec to not be so different that it's painful to convert to Fedora > standards. The spec has to allow to build regular RPM as well as SCL RPM without any change, just by choosing different build root. And this spec definitely fails to do so, since on RHEL, it would always build as SCL RPM. But let's move the discussion into more appropriate palce :) > Expect a new spec/srpm soon.
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.1.1-5.fc18.src.rpm - Removed scl %if stuff Side note: /etc/openshift/ and /etc/openshift/plugins.d/ are now owned by rubygem-openshift-origin-common
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.5.1-1.fc20.src.rpm - update to latest version - fixed it so it would build on F19+ - fixed all scl stuff.
* No test suite - I see that the test suite is not executed nor is any available upstream. You should definitely do something about it, although it is not blocker. I just need to do this remark, sorry ;) * Macroize /etc - For /etc, we have %{_sysconfdir} macro [1] - Looking at rubygem-openshift-origin-common, there is the same issue - Have you ever considered creating some package, such as in ruby we have rubygems-devel, which would carry definition of some common macros used by OS? For example the /etc/openshift and /etc/openshift/plugins.d paths looks like good candidates for such package. * -doc subpackage should be noarch - Documentation is typically arch independent, isn't it? * -doc subpackage does not own directories - The -doc subpackage depends on the main package typically, in that case, the package requires rubygems and everything is ok. However, if you do not require the main package, you should take care of ownership of the whole path to the documentation, which I don't believe you did. You own just the doc dir under rubygems directory, not the rest. - To solve that, I would suggest to require the main package as we usually do. * -doc subpackage without group - The group is not mandatory anymore, but for documentation, it comes natural to me, that the group should be specified there, but this is not blocker. * Use %gem_install macro - Is there any reason, why you are not using %gem_install macro? Is that due to RHEL? If yes, I'd love to see it conditionalized anyway. * Move Gemfile into -doc subpackage - This file is not needed for runtime, so I would suggest to move it into -doc subpackage (or alternatively just exclude it). * Prefix %{gem_instdir} with %dir - I always suggest to do %dir %{gem_instdir} so you have better control what goes into the package - Currently, I would say (although I did not build the package) that you have duplicated LICENSE COPYRIGHT Gemfile files. One version goes into /usr/share/doc and their second version redisdes in %{gem_instdir}, but I might be mistaken [1] https://fedoraproject.org/wiki/Packaging:RPMMacros
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.5.1-2.fc20.src.rpm You were right, I had taken the upstreams stuff, and overlooked the doc header section as well as the files section. - Reworked files section -- moved Gemfile to -doc section -- prefixed %{gem_instdir} with %dir --- This brought out a bunch of duplicates, as well as extra directories --- Fixes the whole files section up - Reworked the doc section -- added Requires of the main package -- added noarch --- I put this in, but isn't this redundant because the package itself is a noarch? -- added group --- Just for you Vit ;) - Reworked %build section -- Yes, it was originally that way because it is supposed to build on RHEL6. -- It just looks to ugly to have the if statement, and since this will not be going into EPEL6, I did a complete replacement with the %gem_install macro. -- Side note. I would love for that to make it into the RHEL6 rubygems-devel package. It is so much cleaner. - No test -- Yep, upstream has been pretty hard to get standalone tests out of. - Macroize /etc -- This is due to the scl macro's. If we use the %{_sysconfdir} variable, scl changes that from /etc/ to /opt/rh/<version>/root/etc/ while then breaks everything. -- I do like your idea of something like a openshift-origin-devel package that has some variables like %{_openshiftconfdir} %{_openshiftplugindir} %{_openshiftlogdir}
(In reply to Troy Dawson from comment #16) > - Reworked the doc section > -- added noarch > --- I put this in, but isn't this redundant because the package itself is a > noarch? Oh, sorry ... you are right. I did not realized that. My bad. * Is the %{gem_instdir}/conf worth of keeping? - Since its content is already copied into /etc, is the folder worth of keeping? Just asking ... > - Reworked %build section > -- Yes, it was originally that way because it is supposed to build on RHEL6. > -- It just looks to ugly to have the if statement, and since this will not > be going into EPEL6, I did a complete replacement with the %gem_install > macro. Well, you can always define the macro on top of the .spec file, if not available. That is IMO a bit better approach, since you don't pollute whole .spec file with ifdefs, there would be just one section on top. > -- Side note. I would love for that to make it into the RHEL6 > rubygems-devel package. It is so much cleaner. Yay, me too. I am trying to push PMs as much as I can, but still not sure if my effort will be successful in near future :/ > - Macroize /etc > -- This is due to the scl macro's. If we use the %{_sysconfdir} variable, > scl changes that from /etc/ to /opt/rh/<version>/root/etc/ while then breaks > everything. As guidelines strongly encourages usage of macros [1], I must strongly encourage you to use them as well. BTW, are you aware of %{_root_sysconfdir} SCL macro? You might try to redefine %{_sysconfdir} back to %{_root_sysconfdir} for SCL build or something less magical, i.e. define new constant which will differ for SCL and non-SCL builds. Actually, on second look, you are installing there just some .example file, not actual configuration, so not sure if it wouldn't be better to entirely forget about installing it into /etc and you would have this issue solved as well as the %{gem_instdir}/conf issue and you would get rid of "rubygem-openshift-origin-auth-kerberos-doc.noarch: W: non-conffile-in-etc /etc/openshift/plugins.d/openshift-origin-auth-kerberos.conf.example" rpmlint warning. * The runtime dependencies are not fulfilled - I observe following error: irb(main):002:0> require 'openshift-origin-auth-kerberos' Gem::LoadError: Could not find 'moped' (~> 1.4.2) - did find: [moped-1.5.0] - It seems that we have in fedora newer version of rubygem-moped then OO needs. * Description - Isn't the description too brief? It even does not mention OpenShift, although one could deduce it from the package name. Please consider to provide better description. [1] https://fedoraproject.org/wiki/Packaging:Guidelines#Macros
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.5.1-3.fc20.src.rpm Sorry for the delay, I was on vacation. - Description, added that it is for OpenShift - macro for /etc/ -- In the end I went with your suggestion of not having it in that directory at all. It is an example file and it is already in a different place in the filesystem. - moped depedancy -- There is nothing I can do in this package to fix the moped dependency problem. Moped is not mentioned in any code, gemfile, or comments in this package. This is a problem with mongoid. I am the maintainer for that package as well and I am working on it. https://bugzilla.redhat.com/show_bug.cgi?id=980526 -- Although the dependancy does affect this package, I believe it can be approved while the problem is getting fixed, because it will not affect the spec file, or final rpm build of this package.
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.10.2.1-1.fc20.src.rpm Updated to the latest upstream version.
What's the latest on this review?
A few issue to fix, once they are fixed, i will approve the package. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Package installs properly. This part couldn't be tested because of a bug in openshift-broker rpm, see #1033928 - mocha is not really used or needed, so it should be dropped from Requires - same goes for json, not used anywhere in the code, should be dropped in gemspec and Requires ===== 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. Note: There is no build directory. Running licensecheck on vanilla upstream sources. No licenses found. Please check the source files for licenses manually. [x]: License file installed when any subpackage combination is installed. [x]: Package must own all directories that it creates. [x]: Package contains no bundled libraries without FPC exception [x]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf %{buildroot} present but not required [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]: 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: No rpmlint messages. [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 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]: 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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local Ruby: [x]: Platform dependent files must all go under %{gem_extdir_mri}, platform independent under %{gem_dir}. [x]: Gem package must not define a non-gem subpackage [x]: Macro %{gem_extdir} is deprecated. [x]: Gem package is named rubygem-%{gem_name} [x]: Package contains BuildRequires: rubygems-devel. [x]: Gem package must define %{gem_name} macro. [x]: Pure Ruby package must be built as noarch [x]: Package does not contain Requires: ruby(abi). [x]: Package contains Requires: ruby(release). ===== 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). [-]: 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. [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]: 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]: Dist tag is present (not strictly required in GL). [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. Ruby: [!]: Test suite of the library should be run. [x]: Specfile should use macros from rubygem-devel package. Note: The specfile doesn't use these macros: %{gem_libdir}, %{gem_spec}, %exclude %{gem_cache} [x]: Gem package should exclude cached Gem. [x]: Gem should use %gem_install macro. ===== EXTRA items ===== Generic: [!]: Rpmlint is run on all installed packages. Note: Mock build failed See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: rubygem-openshift-origin-auth-kerberos-1.10.2.1-1.fc20.noarch.rpm rubygem-openshift-origin-auth-kerberos-doc-1.10.2.1-1.fc20.noarch.rpm rubygem-openshift-origin-auth-kerberos-1.10.2.1-1.fc20.src.rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Requires -------- rubygem-openshift-origin-auth-kerberos (rpmlib, GLIBC filtered): openshift-origin-broker ruby(release) rubygem(json) rubygem(krb5-auth) rubygem(mocha) rubygem(openshift-origin-common) rubygem(openshift-origin-controller) rubygems rubygem-openshift-origin-auth-kerberos-doc (rpmlib, GLIBC filtered): rubygem-openshift-origin-auth-kerberos Provides -------- rubygem-openshift-origin-auth-kerberos: rubygem(openshift-origin-auth-kerberos) rubygem-openshift-origin-auth-kerberos rubygem-openshift-origin-auth-kerberos-doc: rubygem-openshift-origin-auth-kerberos-doc Source checksums ---------------- http://mirror.openshift.com/pub/openshift-origin/source/rubygem-openshift-origin-auth-kerberos/openshift-origin-auth-kerberos-1.10.2.1.gem : CHECKSUM(SHA256) this package : fbadb7ca0f31e265cefbac857cf88f6eb74d7cde6bb4e630982b7452fdae0e6b CHECKSUM(SHA256) upstream package : fbadb7ca0f31e265cefbac857cf88f6eb74d7cde6bb4e630982b7452fdae0e6b Generated by fedora-review 0.4.0 (cf29f98) last change: 2013-02-08 Command line :./try-fedora-review -b 894482 Buildroot used: fedora-20-x86_64 Active plugins: Generic, Ruby, Shell-api Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG
See https://github.com/openshift/origin-server/pull/4242 also, not sure if we want to keep in sync the spec file, and how
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.10.2.1-2.fc20.src.rpm - Fixed bug #1033928 -- Put in an override, so you should be able to test it now. - Removed mocha and json from gemspec and spec file -- Will work with upstream on this. In the pull request you showed, they only removed it from the gemspec file. -- We are only tracking the upstream stable releases in Fedora. The unstable releases have too much churn in them to keep up.
Spec URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos.spec SRPM URL: http://tdawson.fedorapeople.org/openshift-origin/rubygem-openshift-origin-auth-kerberos-1.15.1.1-1.fc20.src.rpm - Updated to latest stable, 1.15.1.1 - If you feel this is good, I will work on getting all the changes, including gemspec, into upstream.
Closing Review Request. The latest versions of this package is no longer supported on Fedora, and is not expected to be in the future. Thank you for your efforts in reviewing this package up to this point.