Fedora Merge Review: acpid http://cvs.fedora.redhat.com/viewcvs/devel/acpid/
I'll review this package.
OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (GPL) OK - License field in spec matches See below - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: 3aff94e92186e99ed5fd6dcee2db7c74 acpid-1.0.4.tar.gz 3aff94e92186e99ed5fd6dcee2db7c74 acpid-1.0.4.tar.gz.1 OK - Package needs ExcludeArch OK - BuildRequires correct OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. See below - Package has correct buildroot OK - Package is code or permissible content. OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. See below - No rpmlint output. OK - final provides and requires are sane: SHOULD Items: OK - Should build in mock. OK - Should build on all supported archs OK - Should function as described. OK - Should have sane scriptlets. See below - Should have dist tag OK - Should package latest version 10 bugs - check for outstanding bugs on package. Issues: 1. Buildroot should be changed to standard. Should add smp_mflags? 2. Might include COPYING, README, Changelog, TODO as doc files? 3. rpmlint our pal says: rpmlint on ./acpid-1.0.4-5.i386.rpm W: acpid conffile-without-noreplace-flag /etc/acpi/events/power.conf W: acpid conffile-without-noreplace-flag /etc/acpi/events/video.conf W: acpid conffile-without-noreplace-flag /etc/logrotate.d/acpid Should all be noreplace? E: acpid non-readable /usr/sbin/acpid 0750 E: acpid non-standard-executable-perm /usr/sbin/acpid 0750 Should this really be non readable by anyone? Why? If so, perhaps a rpmlint bug should be filed? W: acpid service-default-enabled /etc/rc.d/init.d/acpid Should this really be enabled on all machines? Are there cases where it might not be desired by default? rpmlint on ./acpid-1.0.4-5.src.rpm W: acpid strange-permission acpid.init 0755 W: acpid prereq-use /sbin/chkconfig, /sbin/service Should perhaps be: Requires(post): /sbin/chkconfig Requires(preun): /sbin/chkconfig Requires(preun): /sbin/service See: http://www.fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-a6d7a1ed9d77dbb8d4af067378a79b838aebb20a W: acpid setup-not-quiet Should add -q to setup. 4. In the files section: %verify(not md5 size mtime) %ghost %config(missingok,noreplace) /var/log/acpid Why all this? /usr/bin/acpi_listen /usr/sbin/acpid Should those have %{_bindir} and %{_sbindir} ? /usr/share/man/man8/acpid.8.gz /usr/share/man/man8/acpi_listen.8.gz Should have %{_mandir} ? 5. You might look at the outstanding bugs on this package. In particular the bugs asking for better scripts might stand to have a response like "please submit your outstanding scripts for inclusion"
1: Fixed Buildroot, add smp_flags. Also added the %{?dist} to the Release 2: Included doc files 3.1: Checked, all 3 should really be noreplace, fixed. 3.2: acpid can't be run as anything else but root, so there is no point in it being executable or readable by everyone. 3.3: acpid isn't a network/remote service, so enabling it by default is safe. Also even if a machine doesn't have ACPI (nowadays kinda rare, but still), making it default will be the best choice the the majority of systems. 3.4: Why is the premission for an initscript strange with 0755, even in a srpm? But fixed the PreReqs to use the correct modern style. 3.5: Added -q for %setup 4.1: Typical /var/log/FOO entry. The package itself doesn't package a real file, but "supports" one being there. See other /var/log logfile supporting packages. 4.2: Fixed. Also fixed all the /etc occurences with %{_sysconfdir} and /usr/share with %{_datadir} 4.3: Fixed. Thanks for the review, Read ya, Phil
Thanks for looking at those items... I am reassigning this back to me. I will hopefully be able to take a look later today.
Thanks for the quick fixes and reply... 1. good. ok. 2. good. ok. 3.1 good. ok. 3.2 I suppose so. ok. 3.3 good. ok. 3.4 Not sure why it's complaining about that 0755 init script. ;( good on the prereqs. 3.5 good. ok. 4.1 Humm...I haven't seen that used for /var/log/FOO files before... I guess there aren't many packages that make such files. Looking at for example vixie-cron, it just has no mention of the log file at all. I guess the question is: do we want this package to remove the /var/log/acpid log when the package is removed? Or should it linger around? We should probibly decide what behavior is desired and make all the packages that create /var/log files do the same thing... In fact I am hard pressed to find another package that does things the way this one does. Do you have any example? scrollkeeper appears to ghost the log file, but yum, x.org, vixie-cron just don't claim anything for their log file (ie, it's unowned). 4.2 good. ok. 4.3 good. ok. Setting it back to you to comment on the 4.1 issue. When you do, please assign it back to me and change the fedora-review flag back to ? Thanks again for the quick fixes...
Hm. I've looked at other packages with logfiles in /var/log, and several actually owned those files: pam: %ghost %verify(not md5 size mtime) /var/log/faillog scrollkeeker: %ghost %{_localstatedir}/log/scrollkeeper.log setup: %ghost %attr(0644,root,root) %verify(not md5 size mtime) /var/log/lastlog So there are a few other examples where the logfile is at least being referenced by a package using a %ghost entry. I mean, it boils basically down to what files a package should own. And typically (and historically) those consisted of the files that were clearly and distinctly connected to that package. There could be the packaged files or, like e.g. for /var/cache files created during the lifetime of a package on a system. And imo /var/log/acpid is pretty clearly connected to one specific package, namely acpid. ;) Maybe this point should really be brought up at the next Fedora meeting to see what the opinion of others are on it. The above is just my personal view after maintaining packages for quite some time, but i'd have no problem if the general rule would be to keep /var/log files unreferenced. Read ya, Phil
Hey Phil. Did we come to any conclusion based on the thread on Fedora Maintainers? Or would you like me to try and get the packaging comittee to come up with a guideline for this case? Personally, I think this package should not have any ownership of the log file. Since logrotate will leave other versions of the log around, something is going to be lingering on package removal anyhow. I guess a case could be made for making a /var/log/acpid/ directory and putting logs in there, as that would make selinux labeling easier, but thats all I can think of. Your thoughts?
Hi Kevin. After the discussion on f-m list i completely agree with your opinion that the logfile shouldn't be owned at all, partially because of the logrotate reasons but also because it doesn't really make sense if a lot of packages don't own the logfiles because of undecidable situtions and just a few do. It would and should be different with directories in /var/log/, but acpid doesn't have one. I'll rebuild the package without the ownership of the logfile today. That would finish the review then iirc, right? :) And thanks again for the good review, work and the really helpful comments. This way we can improve the review guidelines as well, so everyone can benefit from it. Read ya, Phil
In reply to comment #8: > I'll rebuild the package without the ownership of the logfile today. Sounds good. > That would finish the review then iirc, right? :) Yeah, thats the last blocking issue I see. >And thanks again for the good review, work and the really helpful comments. >This way we can improve the review guidelines as well, so everyone can benefit >from it. Thank you for quick fixes and good replies. :)
Sorry I didn't get back to this until now. I see you made the changes for the log file, so all looks good... This package is APPROVED. Per the new offical review process, I will leave it assigned to me with the review flag as + (approved). Feel free to close it RAWHIDE now since it's been pushed out...
Ok, closing it as RAWHIDE. Read ya, Phil