Bug 225769 - Merge Review: freeradius
Merge Review: freeradius
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Package Reviews List
:
Depends On:
Blocks: 487059
  Show dependency treegraph
 
Reported: 2007-01-31 13:40 EST by Nobody's working on this, feel free to take it
Modified: 2009-12-22 00:17 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-22 00:17:30 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lemenkov: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:40:04 EST
Fedora Merge Review: freeradius

http://cvs.fedora.redhat.com/viewcvs/devel/freeradius/
Initial Owner: twoerner@redhat.com
Comment 1 Karsten Hopp 2007-02-23 09:26:54 EST
freeradius-1.1.3-3 prepared with fixes for the most common review issues
Comment 2 Peter Lemenkov 2009-04-21 10:59:20 EDT
I'll review it
Comment 3 Thomas Woerner 2009-04-22 03:46:20 EDT
jdennis is the maintainer, now.
Comment 4 Peter Lemenkov 2009-07-27 08:11:27 EDT
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.
Comment 5 Peter Lemenkov 2009-07-31 04:21:46 EDT
Ping.
Comment 6 John Dennis 2009-08-15 12:18:58 EDT
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.
Comment 7 Peter Lemenkov 2009-12-21 06:02:59 EST
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.
Comment 8 John Dennis 2009-12-21 14:01:04 EST
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)
Comment 9 Peter Lemenkov 2009-12-21 15:28:50 EST
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.
Comment 10 Peter Lemenkov 2009-12-22 00:17:30 EST
Since all changes now imported to cvs tree, I'm closing this.

Note You need to log in before you can comment on or make changes to this bug.