Fedora Merge Review: freeradius http://cvs.fedora.redhat.com/viewcvs/devel/freeradius/ Initial Owner: twoerner
freeradius-1.1.3-3 prepared with fixes for the most common review issues
I'll review it
jdennis is the maintainer, now.
REVIEW (sorry for the delay): +/- rpmlint outputs a LOT of messages: http://peter.fedorapeople.org/freeradius_rpmlint.txt However, as John Dennis already mentioned in bz#487059, most of these issues may be omitted safely: * non-standard GID: * non-readable files * non-config files under /etc * no-documentation The rest of rpmlint messages: freeradius.i586: W: obsolete-not-provided freeradius-dialupadmin freeradius.i586: W: obsolete-not-provided freeradius-dialupadmin-ldap freeradius.i586: W: obsolete-not-provided freeradius-dialupadmin-mysql freeradius.i586: W: obsolete-not-provided freeradius-dialupadmin-postgresql I think, this may be omitted too. freeradius.i586: E: zero-length /var/log/radius/radius.log Likewise. freeradius.i586: E: incoherent-logrotate-file /etc/logrotate.d/radiusd Due to historical reasons, the main application name differs from the package's name. We cannot change anything here. So this issue should be omitted. freeradius.i586: W: file-not-utf8 /usr/share/doc/freeradius-2.1.6/rfc/pppext-eap-sim-12.txt This should be fixed at the %prep stage. freeradius.i586: E: zero-length /var/log/radius/radutmp May be omitted. freeradius.i586: W: file-not-utf8 /usr/share/doc/freeradius-2.1.6/rlm_dbm This should be fixed at the %prep stage. freeradius.i586: E: executable-marked-as-config-file /etc/rc.d/init.d/radiusd Does it really should be marked as a config? I'm sure it doesn't. All installation-specific changes should be placed in /etc/sysconfig/%{name} or something similar. freeradius.i586: W: file-not-utf8 /usr/share/doc/freeradius-2.1.6/rfc/draft-sterman-aaa-sip-00.txt This should be fixed at the %prep stage. freeradius.i586: E: non-standard-executable-perm /etc/raddb/certs/bootstrap 0750 I don't like the idea to place executables (scripts) into /etc/ (exept known exeptions like /etc/init.d ). However looks like this should be placed here due to historical reasons. So this issue may be omitted too. freeradius.i586: W: incoherent-init-script-name radiusd ('freeradius', 'freeradiusd') Due to historical reasons, the main application name differs from the package's name. We cannot change anything here. So this issue should be omitted. freeradius-debuginfo.i586: W: hidden-file-or-dir /usr/src/debug/freeradius-server-2.1.6/src/main/.libs freeradius-debuginfo.i586: W: hidden-file-or-dir /usr/src/debug/freeradius-server-2.1.6/src/main/.libs Looks like a leftover from wrongly picked up binaries byt script to generate debuginfo. I think this may be ignored. freeradius-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/freeradius-server-2.1.6/src/modules/rlm_ippool/rlm_ippool_tool.c freeradius-debuginfo.i586: W: spurious-executable-perm /usr/src/debug/freeradius-server-2.1.6/src/modules/rlm_linelog/rlm_linelog.c This should be fixed at the %prep stage. freeradius-postgresql.i586: W: summary-not-capitalized postgresql support for freeradius freeradius-unixODBC.i586: W: summary-not-capitalized unixODBC support for freeradius This may be omitted + The package is named according to the Package Naming Guidelines . + The spec file name matches the base package %{name}, in the format %{name}.spec . +/- The package meets the Packaging Guidelines. At least, I cannot find any issues. Yet, some small issues still remains: * I'm sure, that %description for -devel subpackage must be changed, since it has nothing to do with static libraries. * See my note regarding ldconfig below. * I advice you to consider adding support for Firebird and iodbc, since both of them are included in Fedora. However this is not a blocker - just my suggestion. + The package is licensed with a Fedora approved license and meets the Licensing Guidelines . + The License field in the package spec file matches the actual license. + The file, containing the text of the license(s) for the package, is included in %doc. + The spec file is written in American English. + The spec file for the package is legible (as legible, as this quite complex package can be). + The sources used to build the package match the upstream source, as provided in the spec URL. + The package successfully compiles and builds into binary rpms on at least one primary architecture. dist-5E-epel http://koji.fedoraproject.org/koji/taskinfo?taskID=1536573 dist-f11 http://koji.fedoraproject.org/koji/taskinfo?taskID=1536583 + All build dependencies are listed in BuildRequires. - The main package calls ldconfig in %post and %postun, however I didn't see any library objects stored in the default ldconfig paths. For me it seems like the leftofver. Please, correct me if I wrong. + The package owns all directories that it creates. + The package does not list a file more than once in the spec file's %files listings. + Permissions on files are set properly. Note, that freeradius is a security-sensitive application, so many files marked as non-readable for non-privileged users. + The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). + The package consistently uses macros. + The package contains code, or permissible content. + No extremely large documentation files. + Everything, the package includes as %doc, does not affect the runtime of the application. + The C header files are in a -devel subpackage. + No static libraries. + No pkgconfig(.pc) files. +/- In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}, but freeradius-devel requires freeradius-libs instead. However, I suspect, that there is some rationale behind this. Actually, I suspect also, that something is broken here, since these two libraries located not in any of the standart ldconfig paths. + The package does NOT contain any .la libtool archives. + The package does not own files or directories already owned by other packages. + At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT). + All filenames in rpm packages are valid UTF-8. Please, comment the issues, noted above, and I'll continue.
Ping.
Thank you for your review. My apologies for the delay, I have been on vacation (actually I'm still on vacation) and will return to work Monday 8/17. I expect to be swamped the first few days back, however I will try to get to this as soon as possible.
Latest scratchbuild: http://koji.fedoraproject.org/koji/taskinfo?taskID=1882750 As usual for such oldschool packages, there is a large list of rpmlint's complaints and warnings (most of them may be safely ignored in this particular case): http://peter.fedorapeople.org/freeradius-2.1.7-5_rpmlint.txt Here is a diff over my last rpmlint check: http://peter.fedorapeople.org/freeradius-2.1.7-5.diff I removed safe-to-ignore rpmlint messages, and here is a final report (I added numbers for the simplicity): [petro@Workplace i386]$ cat ~/fuse/sshfs/sulaco/home/petro/fuse/sshfs/fedora/public_html/freeradius-2.1.7-5_rpmlint.txt |grep -v non-readable|grep -v non-standard-dir-perm|grep -v no-documentation -n 1:freeradius.i686: W: incoherent-version-in-changelog 2.1.7-4 ['2.1.7-5.fc12', '2.1.7-5'] 2:freeradius.i686: W: file-not-utf8 /usr/share/doc/freeradius-2.1.7/rfc/draft-sterman-aaa-sip-00.txt 3:freeradius.i686: E: incoherent-logrotate-file /etc/logrotate.d/radiusd 4:freeradius.i686: W: file-not-utf8 /usr/share/doc/freeradius-2.1.7/rlm_dbm 5:freeradius.i686: W: file-not-utf8 /usr/share/doc/freeradius-2.1.7/rfc/pppext-eap-sim-12.txt 6:freeradius.i686: E: non-standard-executable-perm /etc/raddb/certs/bootstrap 0750 7:freeradius.i686: W: incoherent-init-script-name radiusd ('freeradius', 'freeradiusd') 8:freeradius-debuginfo.i686: W: spurious-executable-perm /usr/src/debug/freeradius-server-2.1.7/src/modules/rlm_ippool/rlm_ippool_tool.c 9:freeradius-debuginfo.i686: W: spurious-executable-perm /usr/src/debug/freeradius-server-2.1.7/src/modules/rlm_linelog/rlm_linelog.c 10:freeradius-debuginfo.i686: W: hidden-file-or-dir /usr/src/debug/freeradius-server-2.1.7/src/main/.libs 11:freeradius-debuginfo.i686: W: hidden-file-or-dir /usr/src/debug/freeradius-server-2.1.7/src/main/.libs 21:12 packages and 0 specfiles checked; 122 errors, 18 warnings. [petro@Workplace i386]$ 1. Easyfix - you simply forgot to add %changelog entry for the latest rebuild. 2. I can't find easily what encoding this file is encoded with. If you know, then, please. add convert it from this codepage to UTF-8 at %prep stage. 3. Due to historical reasons, the main application name differs from the package's name. We cannot change too much here, so this issue should be omitted. 4. Please, add iconv -f iso8859-1 to the %prep stage. 5. The same as for p.2. If you know how to convert it to UTF-8, please do it at the %prep stage. Otherwise simply postpone fixing it to the (not so) nearest future. 6. See my comment above. In short - looks like this should be placed here due to historical reasons. So this issue may be omitted too. 7. Likewise. 8. Please, change permissions to 0644 at the %prep stage. 9. Likewise. 10. I think this may be ignored. 11. Likewise. See also this link (for those, who interested in John's PoV on these issues): https://bugzilla.redhat.com/show_bug.cgi?id=487059#c3 I mostly agree with his reasons, however I'd like to see #1,8,9 fixed. Ok, that's all about rpmlint messages. Also I've got some complaints regarding various aspects iof the package itself: * The *-devel package's description is irrelevant to the sub-package's content. It doesn't contain any static libraries - just a C header files. * I don't see any development libraries in *devel sub-package (in fact I don't see any librarties in the common ldconfig paths, but that will be my next question). I suspect, that shared library objects in %{_libdir}/freeradius/ , which doesn't contain 2.1.7 in their names, should be placed in *-devel sub-package, but I'm not sure. Could you, please, comment on this. * As I already mentioned, I don;'t see any shared library objects, visible to ldconfig. So I pretty sure there is no need to run ldconfig. * As I told in my previous comment. I don't see rationale behind splitting main package to freeradius and freeradius-libs. Could you, please, comment on this. Citing myself: "In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}, but freeradius-devel requires freeradius-libs instead. However, I suspect, that there is some rationale behind this. Actually, I suspect also, that something is broken here, since these two libraries located not in any of the standart ldconfig paths." * And one more - I suggestion you to enable modules for dealing with firebird and iodbc (both are available in Fedora). Obviously, this only applies to Fedora. That's all so far.
Thank you for your careful review Peter. You're correct, there was no need for the freeradius-libs subpackage, I can't recall why it existed, possibly it was historic, I've removed it and added an Obsoletes for it. The subpackage dependencies were also upadated. You're correct, ldconfig is unnecessary. It's presence was part historic and part defensive. A somewhat common scenario with FreeRADIUS newbies is to grab the upstream tarball and build and install it themselves. They run into a number of problems because they didn't build and install it correctly so they query the support forum and are told to use prebuilt binaries, so they install the rpm's but they still have problems because they messed up ld from their initial unsuccessful attempts, then they are told to run ldconfig which clears up the problem for them. That's why running ldconfig was somewhat "defensive". However, your point about it being unnecessary is valid so I've removed it. I fixed the devel subpackage description. I removed the execute permissions on the source files. As for the 3 documentation files not being utf-8. The rlm_dbm and pppext-eap-sim-12 are getting triggered only for a contributor's name in the file. I spent about an hour trying to figure out what encoding these files were using but I couldn't, it'd definitely not iso-8859-1. If you use iconv to convert from iso-8859-1 to utf-8 on these files you'll discover they're not converted correctly. Since iconv doesn't produce the right result and between the two files it's only 3 characters in total which are bogus I don't see a point on running it if the only point is to eliminate the rpmlint message. The draft-sterman-aaa-sip-00.txt file is triggering the warning because the hyphen character used to break words during pagination is 0xED, it occurs only at the end of a line when a word is broken, it really doesn't affect ones ability to read the file, the errors occur on each copy of the file which showed up with a google search. It's within my comfort zone to ignore these utf-8 errors. These changes appear in the 2.1.7-6 version (not yet tagged or built, but in CVS)
Great. You fixed all blocking issues, and I have only few remaining non-blocking suggestions: * Since all sub-packages are requiring main freeradius package now, there is no need to try to create user/group radiusd at %pre stage while installing of any sub-package except main one (freeradius). * There is no need to explicitly mark man-pages as %doc * I'd like to introduce you the hidden directory creation power of 'install' utility. Instead of typing mkdir -p /my/path/ install file.ext /my/path/newfile.ext you may simply type install -D file.ext /my/path/newfile.ext and 'install' will create /my/path/ for you :) Anyway, these my suggestions won't dramatically change end user's experience with freeradius, so it's up to you to decide, whether or not to follow them. This package is APPROVED.
Since all changes now imported to cvs tree, I'm closing this.