Bug 225608

Summary: Merge Review: basesystem
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: pertusus, pknirsch, redhat-bugzilla, rh-bugzilla
Target Milestone: ---Flags: j: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-01-14 02:01:51 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: 426387    

Description Nobody's working on this, feel free to take it 2007-01-31 17:44:16 UTC
Fedora Merge Review: basesystem

http://cvs.fedora.redhat.com/viewcvs/devel/basesystem/
Initial Owner: pknirsch

Comment 1 Roozbeh Pournader 2007-02-05 15:39:20 UTC
rpmlint output:
  SRPM:
W: basesystem summary-ended-with-dot The skeleton package which defines a simple
Red Hat Linux system.
W: basesystem invalid-license public domain
W: basesystem no-url-tag
W: basesystem prereq-use setup filesystem
W: basesystem hardcoded-path-in-buildroot-tag /var/tmp/basesystem-root
E: basesystem no-cleaning-of-buildroot %install
E: basesystem no-cleaning-of-buildroot %clean
W: basesystem no-%prep-section
W: basesystem no-%build-section
W: basesystem no-%install-section
W: basesystem no-%clean-section
  RPM:
W: basesystem summary-ended-with-dot The skeleton package which defines a simple
Red Hat Linux system.
W: basesystem invalid-license public domain
W: basesystem no-url-tag
W: basesystem no-documentation

Random issues:
* Change "Red Hat Linux" to "Fedora" (both in summary and description). blocker.
* What is the version "8.0"?! I can't say this follows naming guidelines.
* Make release integer (6?).
* Using Prereq is bad. Change to Requires. blocker.
* Capitalize "Public Domain".
* Change BuildRoot to %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
* description says this should be the first package installed on a system, but
the package Prereq's "setup" and "filesystem". Confusing.
* Add empty sections for %prep, %build, %install, and %clean. blocker.
* Theoretically, the %files section should contain a %defattr line.


Comment 2 Patrice Dumas 2007-02-17 20:43:05 UTC
I suggest replacing BuildArchitectures with BuildArch.

Comment 3 Phil Knirsch 2007-03-02 15:20:36 UTC
Sorry it took a while to get to do the fixes.

Here first my comments on the findings:

> W: basesystem summary-ended-with-dot The skeleton package which defines a
simple Red Hat Linux system.

Fixed

> W: basesystem invalid-license public domain

Fixed, now using Public Domain (as requested below).

> W: basesystem no-url-tag

There is now upstream for this package, so the only option would be to either
make it http://www.redhat.com/ or http://www.fedoraproject.org/

> W: basesystem prereq-use setup filesystem

Fixed. Now it's Requires(Pre): setup filesystem

> W: basesystem hardcoded-path-in-buildroot-tag /var/tmp/basesystem-root

Fixed. Using the latest recommended BuildRoot 

> E: basesystem no-cleaning-of-buildroot %install

Fixed. Added empty %install section

> E: basesystem no-cleaning-of-buildroot %clean

Fixed. Added empty %clean section

> W: basesystem no-%prep-section

Fixed. Added empty %prep section

> W: basesystem no-%build-section

Fixed. Added empty %build section

> W: basesystem no-%install-section

Fixed. Added empty %install section

> W: basesystem no-%clean-section

Fixed. Added empty %clean section


  RPM:
> W: basesystem summary-ended-with-dot The skeleton package which defines a
simple Red Hat Linux system.

Fixed. See above.

> W: basesystem invalid-license public domain

Fixed. See above.

> W: basesystem no-url-tag

Possible "fixes" listed above.

> W: basesystem no-documentation

basesystem doesn't have a source, nor does it contain any files. so unecessary.


Random issues:
> * Change "Red Hat Linux" to "Fedora" (both in summary and description). blocker.

Fixed.

> * What is the version "8.0"?! I can't say this follows naming guidelines.

Version of basesystem is arbitrary.

> * Make release integer (6?).

Fixed, but release can (and often will) consiste of X.Y.Z components.

> * Using Prereq is bad. Change to Requires. blocker.

Absolutely agreed. Fixed.

> * Capitalize "Public Domain".

Fixed. See above.

> * Change BuildRoot to
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Fixed. Used the even newer recommended BuildRoot entry as per fedoraproject.org

> * description says this should be the first package installed on a system, but
the package Prereq's "setup" and "filesystem". Confusing.

Uhm, yea. Thats one of the things i'd like to go over with some folks and
discuss on f-d-l. Imo the "correct" order of those 3 packages would be:

  basesystem
  filesystem
  setup

For the simple reason as basesystem (as the description also already says)
should be the first package on a system. Then filesystem, as it creates the
basic directory structure of the system. And third setup, which contains the
basic setup for the system. I think one of the problems though is that in order
to basesystem, filesystem and setup pulled in glibc requires basesystem, which
then in turn pulls in filesystem and setup via the Requires(Pre):

[phil@kfurt tmp]$ rpm -q --whatrequires basesystem
glibc-2.5-9

[phil@kfurt tmp]$ rpm -q --whatrequires filesystem
basesystem-8.0-5.1.1
lockdev-1.0.1-10
SysVinit-2.86-14
mkinitrd-5.1.19.2-1
nautilus-2.16.2-5.el5

[phil@kfurt tmp]$ rpm -q --whatrequires setup
filesystem-2.4.0-1
basesystem-8.0-5.1.1
dump-0.4b41-2.fc6
sendmail-8.13.8-2.el5

* Add empty sections for %prep, %build, %install, and %clean. blocker.

Fixed. Added empty sections for those.

* Theoretically, the %files section should contain a %defattr line.

Fixed. Added it, just for completeness.

Summary: Overall should contain now nearly all recommended fixes. Only 2
questions are:

- What to do with URL? Really not happy about any "arbitrary" URL there.
- Discuss on f-d-l how to go about fixing the requires chain.

Read ya, Phil

Comment 4 Patrice Dumas 2007-03-08 23:28:23 UTC
Maybe I am doing something wrong, but I don't see the changes?

Comment 5 Phil Knirsch 2007-03-09 15:47:02 UTC
Argh. Building package as i write this. :)

Sorry, should appear in devel in the next few days.

Read ya, Phil

Comment 6 Warren Togami 2007-07-12 18:32:14 UTC
It appears that changes were checked in as basesystem-8.1-1.  I don't see any
explcit approval above, so this still requires another check before approval.


Comment 7 Patrice Dumas 2007-11-14 13:31:04 UTC
It looks like (from the build logs) that the build root is in fact
never created.

rpmlint output is in my opinion ignorable.
basesystem.src: E: no-cleaning-of-buildroot %install
basesystem.src: E: no-cleaning-of-buildroot %clean
basesystem.src: W: no-url-tag
basesystem.noarch: W: no-documentation
basesystem.noarch: W: no-url-tag

I think that in the %description, the following is not useful:

 Basesystem should be in every installation of a system, and it
 should never be removed.

since this is rpm/packaging job to do that transparently for the
user. (Moreover I guess that removing this package wouldn't hurt
but I won't check ;-).

Comment 8 Patrice Dumas 2007-11-14 23:30:23 UTC
Maybe adding rootfiles as a Requires to that package could
be considered?

Comment 9 Jason Tibbitts 2008-01-21 23:06:02 UTC
Any progress here?  This is one of the reviews we'd like to have finished before
F9 release.

Comment 10 Patrice Dumas 2008-01-21 23:14:33 UTC
(In reply to comment #8)
> Maybe adding rootfiles as a Requires to that package could
> be considered?

Forget it, it is not a good idea. Truely minimal systems
in chroot shouldn't need root files.

Comment 11 Robert Scheck 2008-01-21 23:22:22 UTC
Sorry, why not removing the package at all? Is there any good reason to keep 
this package?

Comment 12 Patrice Dumas 2008-01-21 23:24:12 UTC
I still think that in the %description, the following is not useful
(and it is wrong, though for weird corner cases):

 Basesystem should be in every installation of a system, and it
 should never be removed.

This is not a blocker though.

Another suggestion would be to remove Fedora from the Summary
and %description, there is nothing fedora specific here.

The summary isn't that good in my opinion. Maybe something along:
Summary: Skeleton package which sets up a simple system

And last I think a comment in the spec would be nice, like

# this package is Required by the libc and so should almost always be
# installed, and its only use is to have the needed Requires(Pre)

So
* rpmlint ignorable
basesystem.src: E: no-cleaning-of-buildroot %install
basesystem.src: E: no-cleaning-of-buildroot %clean

You can clean those errors if you want to, be it only for the sake
of having consistent packaging.

basesystem.src: W: no-url-tag
basesystem.noarch: W: no-documentation
basesystem.noarch: W: no-url-tag
* there is nothing built, no file

Please consider fixing my comments, but they are not blockers.

APPROVED


Comment 13 Patrice Dumas 2008-01-21 23:25:25 UTC
(In reply to comment #11)
> Sorry, why not removing the package at all? Is there any good reason to keep 
> this package?

What do you propose instead? Have the libc depend on setup
and setup depend on filesystem?

Comment 14 Robert Scheck 2008-01-22 07:25:26 UTC
Yes. Why not? Currently filesystem has a "Requires(Pre): setup" which seems to 
be unneeded, so make filesystem the base of everything (this even would be the 
case, if RPM would depend on packaged parent directories). Let setup depend on 
filesystem as it would be, if RPM could handle packaged parent directories and 
then make glibc depend on setup. Suggestions?

Comment 15 Robert Scheck 2008-01-22 07:27:30 UTC
And for upgrades, add an obsoletes for basesystem to filesystem.

Comment 16 Patrice Dumas 2008-01-27 21:13:49 UTC
I think it would be better to decide whether the package is 
needed before granting review. reverting to ?.

Comment 17 Enrico Scholz 2008-04-13 17:38:41 UTC
'Requires(Pre): setup' in filesystem is necessary because filesystem ships
objects owned by non-root users. These users are defined in /etc/passwd provided
by 'setup'.

Comment 18 Phil Knirsch 2008-04-17 15:11:29 UTC
Yea. After looking over the comments here i think the proper order should be:

  basesystem
  setup
  filesystem

Now take glibc into the picture which currently Requires(Pre): basesystem. If
we'd drop basesystem completely but added a

  Obsoletes: basesystem-x-y
  Provides: basesystem-x-y

to setup and we should be set for updates. And newer glibc versions should
change the Requires(Pre): basesystem to Requires(Pre): setup

Sounds reasonable?

Comment 19 Phil Knirsch 2008-04-17 15:17:51 UTC
Actually, glibc should have a Requires(Pre): filesystem to create the dependency
chain properly.

Comment 20 Phil Knirsch 2008-04-17 15:18:56 UTC
Hm, and then the

  Obsoletes: basesystem-x-y
  Provides: basesystem-x-y

needs to move into filesystem as well, too, otherwise older glibc versions would
pull in the wrong package for the chain.


Comment 21 Jason Tibbitts 2008-12-04 19:48:24 UTC
Did anything ever happen with this?  I see the basesystem package is still around in F10 and rawhide.  I can't imagine there are any packaging-related issues left since the package is essentially empty, so there can't be much reason why this review would still be open.

Comment 22 Patrice Dumas 2008-12-04 20:19:05 UTC
I already approved it, but reverted since it seemed pointless to me to close a review if th epackages is meant to be removed.

Comment 23 Robert Scheck 2008-12-04 20:28:04 UTC
IMHO the package still should die...

Comment 24 Jason Tibbitts 2009-01-14 02:01:51 UTC
Honestly, the decision about whether this package should die or not has no bearing on whether what is currently in the distro is packaged correctly.  We can't treat it like a regular package and refuse to allow it in; it's already in, and the purpose of this ticket is to ensure that the packaging is OK.

The packaging is obviously OK; the package has nothing substantive save
  Requires(Pre): setup filesystem
name, version, license, summary, description, buildroot, changelog, are all OK.  There's no source to check, no files to inspect.

As such I'm going to approve it.  Sure, it would be nice if it was killed off, but we could certainly find worse-packaged pieces of historical cruft in the distro.

APPROVED