Bug 144818 - NetworkManager scriplets need to be cleaned up
NetworkManager scriplets need to be cleaned up
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: NetworkManager (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Williams
: EasyFix
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-11 14:05 EST by Matthias Saou
Modified: 2007-11-30 17:10 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-01-13 14:47:36 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Spec file patch (5.57 KB, patch)
2005-01-11 14:43 EST, Matthias Saou
no flags Details | Diff

  None (edit)
Description Matthias Saou 2005-01-11 14:05:11 EST
Description of problem:
NetworkManager scriplets need to be cleaned up, they contain
unnecessary comments and little mistakes.

Version-Release number of selected component (if applicable):
NetworkManager-0.3.2-4.3.cvs20041208

Additional info:

1) There are comments in the spec file around the scriplets, which are
obviously included in the real scriplets... which was most certainly
not intended by the person who included them. For instance, the %preun
scriplet is as follows :

-- snip --

if [ "$1" -ge "1" ]; then
  service NetworkManager condrestart > /dev/null 2>&1
fi


##################################
# Main Package Files
##################################

-- snip --

Note that this block of comment is in the spec file, to introduce the
%files section below...

2) There are small mistakes in the scriplets :
- This should not be :
if ! chkconfig --list | grep NetworkManager | grep on; then
        /sbin/chkconfig --add NetworkManager
        /sbin/chkconfig --level 123456 NetworkManager off
fi
Instead, one wants the "chkconfig" line of the init script to default
all levels to off, and unconditionnaly run that --add line, which will
not change the local changes to the on/offs in various runlevels, if
any. Furthermore, that test with the "chkconfig --list" isn't
redirected, so the line is actually outputted when the package is
installed or upgraded, messy.
- One should make up his mind about integers vs. characeters.
Normally, $1 is an integer passed to the scriplets, so the proper
"test" operands to use are -lt, -le, -eq, -ge or -gt unquoted. If one
decides to quote everything, why not, then only = should be used. But
here we have both wrong :

preuninstall scriptlet (using /bin/sh):
if [ $1 = 0 ]; then
    service NetworkManager stop > /dev/null 2>&1
    /sbin/chkconfig --del NetworkManager
fi


##################################
# Post-uninstall
##################################
postuninstall scriptlet (using /bin/sh):
if [ "$1" -ge "1" ]; then
  service NetworkManager condrestart > /dev/null 2>&1
fi


##################################
# Main Package Files
##################################

I'll try to produce a quick patch to the spec file to correct all of
these.
Comment 1 Dan Williams 2005-01-11 14:14:20 EST
I'd be happy to apply the patch.  The thing to keep in mind is that:

1) If the user _hasn't_ ever used NetworkManager, we don't want it to
be started by default or added with chkconfig.  At this time, users
must explicitly enable NM, so just installing the RPM should _not_ run
NM on startup or add it with chkconfig unless its already there

2) We should not restart NM if its not already running (condrestart)

3) If the user has NM set to run on startup and its currently running,
we should restart NM

The current specfile doesn't do these very well, I'll admit :)
Comment 2 Matthias Saou 2005-01-11 14:27:26 EST
I get those points, but for 1), the proper way of making sure
NetworkManager doesn't get added to run on startup for people who
didn't enable it explicitely is simply by changing this line :

# chkconfig: 345 98 02
to :
# chkconfig: - 98 02

in "initscript/RedHat/NetworkManager" of the sources, as this
represents the runlevels in which to run the service by default.
Making the change upstream, adding a patch to the srpm or adding a
one-liner regexp substitution in %prep or %build are all solutions.
Comment 3 Dan Williams 2005-01-11 14:34:52 EST
Aha, the "-" I was not aware of.  Thanks.
Comment 4 Matthias Saou 2005-01-11 14:43:15 EST
Created attachment 109633 [details]
Spec file patch

Changes :
- Keep consistent between spaces and tabs
- Added URL so that people know where to look for more info
- Remove big blocks of comments that caused trouble in the scriplets (sorry! a
good syntax highlighting will do as well...)
- Added missing BuildRequirements
- Updated %description based on current README
- Remove unneeded explicit dependencies
- Use %find_lang macro for translations and file colouring
- Clean up scriplets
- Add directory default attributes
- Re-organize %files order to increase readability
- Add slashes to the %files lines for directories
- Set the gnome sub-package dbus file as %config like the main one

Feel free to take or throw away any parts separately.
Comment 5 Dan Williams 2005-01-11 15:13:33 EST
One quick comment...  The NetworkManager-gnome package does indeed
require GConf2, since NetworkManagerInfo uses it and links to gconf
libraries.
Comment 6 Matthias Saou 2005-01-11 15:42:29 EST
Absolutely. But this is a typical case of redundant dependency
information, as the library requires/provides that rpm computes will
already carry that information :

$ rpm -q --requires NetworkManager-gnome | grep gconf
libgconf-2.so.4

$ rpm -q --whatprovides libgconf-2.so.4
GConf2-2.8.1-1

Having "GConf2" as an explicit dependency can seem harmless at first
(and mostly is, actually ;-)), but say that the GConf2 package
dissapears because it's been swallowed by a compat-* one or replaced
by a new binary compatible one (think fam vs. gamin)... relying only
on automatic deps makes things easier since as long as the same
libraries are provided somewhere, things won't break.

This was untrue in the "old days" when apt for rpm, yum or even
up2date didn't exist, since an explicit missing dep on "GConf2" lead
more easily to the package required to satisfy the dependency than
"libgconf-2.so.4".

Anyway, just take a look at the output, it should make you feel more
comfortable ;-) :
rpm -q --requires NetworkManager-gnome
Comment 7 Matthias Saou 2005-01-13 14:47:36 EST
I see these changes have been merged in the FC3 update that just got
released, thanks! The dbus_version and hald_version macros are nice
and practical too ;-)

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