Bug 468570
Summary: | Review Request: webmin - new package | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Andy Theuninck <andy> |
Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
Status: | CLOSED NOTABUG | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | astrand, fedora-package-review, kevin, mtasaka, notting |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2009-07-01 23:40:39 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 201449 |
Description
Andy Theuninck
2008-10-26 04:34:52 UTC
I'm not going to review this, but I thought I would offer some comments: - Your spec link appears to be incorrect. ;) - Is this based on the upstream spec? I would suggest you just start over with a clean new spec and match it to fedora guidelines, as the upstream spec has too many horrors to list in it. - This spec seems to suffer many of the same issues as the last webmin submission. See bug 184080 for the list of those. Please fix/address those issues. Additionally, some people there may wish to assist you in getting this in shape. Whoops. CORRECT spec link: http://gohanman.com/rpm/Fedora9/SPECS/webmin.spec It's very loosely based on the upstream SPEC. I gutted out tons of the original %pre/%post mess and made the %files section saner (so it actually includes every file or directory the package creates). I made a few changes to the spec based on suggestions in the linked bug. I'll rebuild the SRPM later (no rpmbuild on my webhost). Changes that are just in the spec, currently: * Vendor tag removed * License changed to GPL * The last few extraneous echos [to stdout] removed I left %clean as [ "%{buildroot}" != "/" ]. I don't see any harm in checking against a possibly catastrophic mistake. I have the backup of /etc/webmin in place still, although I'm copying it to /var/webmin instead of leaving it as a dotfile in /etc. Size-wise, /etc/webmin comes in at 1.1MB on my system. If that's too much space, there's probably no real harm in getting rid of the backup entirely and letting rawhide/new release testers fend for themselves. New SRPM build: http://gohanman.com/rpm/Fedora9/SRPMS/webmin-1.430-2.fc9.src.rpm At this point, rpmlint only has two complaints: * using cp in %pre (to back up /etc/webmin) * using chmod in %post (to make the install log root-only readable) Neither of those actions seems terribly "dangerous" to me, to be honest. * Some general issues on your scriptlets: - First of all, generally rpm * should not treat any files/directories which are not listed in any rpms. * if rpm creates some files when the rpm is to be installed, all files must be listed in the rpm itself So: ------------------------------------------------- %pre tempdir=%{_datadir}/%{name}/installation-log mkdir -p $tempdir ------------------------------------------------- - If this directory is needed, then this directory _must_ be listed in %files list ------------------------------------------------- cp -r --remove-destination %{_sysconfdir}/{%name} %{_datadir}/%{name}/webmin-etc-conf-backup ------------------------------------------------- - (well first of all {%name} is wrong) - Treating %_sysconfdir/%name is wrong because this is not listed in any rpms. rpm should handle files only listed in some rpms. If if this webmin fails if some files exist under %_sysconfdir/%name, then %pre script should simply "exit with 1" and do backup of files under %_sysconfdir/%name must be done directly by sysadmin. ------------------------------------------------- %post ./setup.sh >$tempdir/webmin-setup.out 2>&1 chmod 600 $tempdir/webmin-setup.out ------------------------------------------------- - First of all, in this case $tempdir/webmin-setup.out (and the directory $tempdir itself) must be in %files list - And %attr must be used for this file - And please explain what ./setup.sh does (especially explain what files this setup.sh creates/modifies). Those files must all be listed in %files, too - Also "rpm -V webmin" must not fail with this (i.e. rpm stores the information of the size and md5sum values of the installed files. If this script modifies files listed in %files, then %verify(not size md5 mtime) should be used, for example) ------------------------------------------------- cat >%{_sysconfdir}/%{name}/uninstall.sh <<EOFF ------------------------------------------------- - Again, %_sysconfdir/%name{,uninstall.sh} must be %files list (i.e. this script must be packaged in webmin binary rpm from the beginning instead of being created here) ------------------------------------------------- %preun %{_libexecdir}/%{name}/run-uninstalls.pl ------------------------------------------------- - Also here, please explain what rub-uninstalls.pl does (especially what files this script modifies/deletes/etc) Again all files modified by this script must be in %files list. If it is impossible to list files in %files of this rpm, then calling setup.sh or run-uninstalls.pl must be done by sysadmins by themselves manually and must not be done by this rpm automatically. * Emply %clean ------------------------------------------------ %clean rm -rf %{buildroot} ------------------------------------------------ - must exist. * pre-compiled binaries - Please remove pre-compiled files to make it sure that all files are created from FOSS files such as ------------------------------------------------ *.class *.o *.jar *.dll * pre-compiled executable ------------------------------------------------ at %prep It's been over half a year since Mamoru's comments and there's been no response. I guess I'll close this soon if there's no further progress. No response; closing. |