Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 468570 - Review Request: webmin - new package
Summary: Review Request: webmin - new package
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2008-10-26 04:34 UTC by Andy Theuninck
Modified: 2009-07-01 23:40 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-07-01 23:40:39 UTC


Attachments (Terms of Use)

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.


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