Bug 468570

Summary: Review Request: webmin - new package
Product: [Fedora] Fedora Reporter: Andy Theuninck <andy>
Component: Package ReviewAssignee: 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: rawhideCC: 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
Spec URL: http://gohanman.com/rpm/Fedora9/SPECS/python-sybase.spec
SRPM URL: http://gohanman.com/rpm/Fedora9/SRPMS/webmin-1.430-1.fc9.src.rpm
Description: webmin is perl-based web application for managing various hardware and services.

The upstream version is designed to keep all files within a single directory in /usr/libexec (and there doesn't seem to be much interest in changing that). I'm shifting a lot of files over into /usr/share/webmin and sym-linking back into libexec as needed. The bash scripts to do this (and fix up what should & shouldn't have the executable bit set) take awhile to run, but should be flexible enough to work with new upstream releases with little or no modification.

Comment 1 Kevin Fenzi 2008-10-30 17:49:30 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.

Comment 2 Andy Theuninck 2008-10-30 18:23:34 UTC
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.

Comment 3 Andy Theuninck 2008-10-31 01:04:41 UTC
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.

Comment 4 Mamoru TASAKA 2008-11-07 17:51:16 UTC
* 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

Comment 5 Jason Tibbitts 2009-06-23 19:20:58 UTC
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.

Comment 6 Jason Tibbitts 2009-07-01 23:40:39 UTC
No response; closing.