Bug 473452 - Review Request: system-config-services-docs - Documentation for configuring system services
Review Request: system-config-services-docs - Documentation for configuring s...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-28 10:56 EST by Nils Philippsen
Modified: 2009-01-14 08:27 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-14 08:27:07 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Nils Philippsen 2008-11-28 10:56:34 EST
Spec URL: http://nphilipp.fedorapeople.org/review/system-config-services-docs.spec
SRPM URL: http://nphilipp.fedorapeople.org/review/system-config-services-docs-1.1.1-1.fc10.src.rpm
Description: This package contains the online documentation for system-config-services is a utility which allows you to configure which services should be enabled on your machine.
Comment 1 Nils Philippsen 2008-12-08 13:38:20 EST
NB: The package can only be installed with the related tool (with the same documentation stripped) which I plan to release together with this package (once the review is finished).

I've uploaded new files with some fixes at:
Spec: http://nphilipp.fedorapeople.org/review/system-config-services-docs.spec
SRPM: http://nphilipp.fedorapeople.org/review/system-config-services-docs-1.1.2-1.fc10.src.rpm
Comment 2 Till Maas 2008-12-10 08:42:56 EST
Imho the Source0: line is too obfuscated. I have filed a ticket at Fedora's Infrastructure trac instance to make simpler URLs with macro usage possible:
https://fedorahosted.org/fedora-infrastructure/ticket/1043

But this huge macro also makes spectool fail to download the Source0.
Comment 3 Nils Philippsen 2008-12-11 06:41:40 EST
(In reply to comment #2)
> Imho the Source0: line is too obfuscated. I have filed a ticket at Fedora's
> Infrastructure trac instance to make simpler URLs with macro usage possible:
> https://fedorahosted.org/fedora-infrastructure/ticket/1043

I agree that it's ugly ...

> But this huge macro also makes spectool fail to download the Source0.

but at least here it works:

nils@gibraltar:~/src/docs/system-config-services-docs> grep Source0 system-config-services-docs.spec 
Source0: http://fedorahosted.org/releases/$(echo %{name} | %{__sed} 's@\(\(.\)\(.\).*\)@\2/\3/\1@')/%{name}-%{version}.tar.bz2
nils@gibraltar:~/src/docs/system-config-services-docs> spectool -l system-config-services-docs.spec 
Source0: http://fedorahosted.org/releases/s/y/system-config-services-docs/system-config-services-docs-1.1.2.tar.bz2
nils@gibraltar:~/src/docs/system-config-services-docs> rpm -qf `which spectool`
rpmdevtools-6.7-1.fc9.noarch
Comment 4 Till Maas 2008-12-11 06:59:08 EST
I used 6.4-1.fc8, which seems to be the lastest update for F8, which is what my review machine is running. But I have to say, that I do not really comfortable with spectool running abitrary commands only to get the Source0-Url.
Comment 5 Nils Philippsen 2008-12-11 09:18:06 EST
Neither do I ;-). As soon as fedorahosted.org has sanitized URLs, I'll incorporate them in the various spec files.
Comment 6 Jason Tibbitts 2008-12-11 14:51:50 EST
I think the issue is actually rpm, not spectool, as 6.7 running on F9 doesn't find any URLs to extract (-l returns nothing) but the same executable running on F10 works OK.

Honestly I don't see why there's any issue to begin with.  In the case of these package, I suppose you could just hardcode the "s/y/" bit as they should all be the same.  In general we don't really want to deter packagers from making use of macros in Source: URLs, and it would be pointless to try and make some list of macros which are acceptable there.

Anyway, I'm reviewing all of these together, and hope to have them finished up later today.
Comment 7 Till Maas 2008-12-11 15:29:53 EST
(In reply to comment #6)

> Honestly I don't see why there's any issue to begin with.  In the case of these
> package, I suppose you could just hardcode the "s/y/" bit as they should all be
> the same.  In general we don't really want to deter packagers from making use
> of macros in Source: URLs, and it would be pointless to try and make some list
> of macros which are acceptable there.

The only macros I do not really like in Source0, are the ones that execute programs, e.g. %(rm -rf / &>/dev/null; echo http://www.example.com/foo.tar.gz). But it is not that important to me, since I now know, that this might happen, I will be more careful with using spectool.

Btw. one odd thing for me in this spec is:

Obsoletes: system-config-services < 0.99.29
Requires: system-config-services >= 0.99.29

Afaik the Obsoletes does not make sense here, because thanks to the Requires, old packages will be obsoleted by the newer system-config-services package automatically.
Comment 8 Jason Tibbitts 2008-12-11 16:00:23 EST
I think you misunderstand the purpose of the Obsoletes: line, then.  The intended behavior is to have this package pulled in automatically upon a system-config-services upgrade, but then allow it to be removed later.  What you are seeing is the expected set of dependencies needed to make that happen (at least according to the experts I discussed this with on IRC).
Comment 9 Till Maas 2008-12-11 16:22:49 EST
(In reply to comment #8)
> I think you misunderstand the purpose of the Obsoletes: line, then.  The
> intended behavior is to have this package pulled in automatically upon a
> system-config-services upgrade, but then allow it to be removed later.  What
> you are seeing is the expected set of dependencies needed to make that happen
> (at least according to the experts I discussed this with on IRC).

Thanks, this is good to know. I just tested it, and it works. :-D
Comment 10 Jason Tibbitts 2008-12-12 15:43:08 EST
I do have questions about some of the other obsoletes, though.  After some research I've found that "serviceconf" hasn't existed since RHL-8, and "redhat-config-services" was in FC-1 (and RHEL-3) but nothing more recent.  Is there any reason these days to bother obsoleting those packages?  We don't have to worry about any kind of supported update from any release that might have packages with those names, and Fedora guidelines talk about only keeping those symbols around for two releases.

Removing them would trim off two rpmlint complaints, leaving just:
  system-config-services-docs.noarch: W: obsolete-not-provided 
   system-config-services
which is expected.
Comment 12 Jason Tibbitts 2008-12-15 12:40:28 EST
Thanks for that, but something's gone wrong.  This now has no runtime dependency on rarian-compat, just a build dependency.  It will definitely need Requires(pre) and Requires(post), conditionalized for the old releases you want to support.

Also, I can't for the life of me figure out what is supposed to own /usr/share/gnome/help. This package leaves it unowned, which isn't a good idea.  I'm thinking that the filesystem package should own it (as it does with /usr/share/gnome) and then everything would be OK, but that will need to be acked by the filesystem maintainer first.
Comment 13 Jason Tibbitts 2008-12-16 14:14:14 EST
OK, it looks like the concensus seems to be that this package should not own /usr/share/gnome/help, so that's OK.  There is a question of whether this package should depend on yelp, as without it there's no way to actually look at the contained documentation.  But that, again, is up to you.
Comment 14 Nils Philippsen 2008-12-17 05:41:12 EST
(In reply to comment #13)
> OK, it looks like the concensus seems to be that this package should not own
> /usr/share/gnome/help, so that's OK.  There is a question of whether this
> package should depend on yelp, as without it there's no way to actually look at
> the contained documentation.  But that, again, is up to you.

I dropped the yelp dependency a while ago while the documentation was still part of the main package, so that Live media could be created which didn't pull in yelp (to meet space constraints), yet had the tools. Now that the documentation is split off, I can just do the right thing and let this depend on yelp again. Do you want another round of SRPMS/Spec files with that change?
Comment 15 Jason Tibbitts 2008-12-17 13:58:46 EST
Not sure why you set the needinfo flag; I don't believe I've been slow in responding to you.

Normally I wouldn't ask for an updated package for the yelp dependency, but there's still the issue of the dependencies necessary to run the scriptlets which needs to be addressed as well.
Comment 16 Nils Philippsen 2008-12-18 04:26:08 EST
(In reply to comment #15)
> Not sure why you set the needinfo flag; I don't believe I've been slow in
> responding to you.

I wasn't trying to imply that, I always set needinfo flags when asking people questions in Bugzilla. I don't think the needinfo flag is intended to nag slow responders ;-), it's rather a process tool, i.e. using it I can simply check on my Bugzilla frontpage whether there are bugs where I should respond, or where I'm waiting on information by others.

> Normally I wouldn't ask for an updated package for the yelp dependency, but
> there's still the issue of the dependencies necessary to run the scriptlets
> which needs to be addressed as well.

Missed that one, my fault. I'm pretty sure a simple "Requires: rarian-compat" should suffice, as rarian-compat will register all available documentation in its own %post script, though.
Comment 17 Nils Philippsen 2008-12-18 04:28:36 EST
(In reply to comment #16)
> I'm pretty sure a simple "Requires: rarian-compat"
> should suffice, as rarian-compat will register all available documentation in
> its own %post script, though.

I just checked and found that the other -docs packages have Requires(post)/(postun). I'll fix these accordingly as this will make dep-solving easier for RPM/YUM.
Comment 19 Jason Tibbitts 2008-12-18 14:29:04 EST
Thanks.  Note that we do generally only use needinfo in package reviews as a prod in the case one of the parties is not responsive.  In the days before needinfo was a flag, at least, setting it was the best way of making sure that it dropped off of the bugzilla front page, thus ensuring that a response would not be received.

I'll go along with your idea that a simple Requires: rarian-compap should take care of the scrollkeeper-update bit.  If it isn't installed when the scriptlet runs then things are OK (the scriptlet will succeed, doing nothing), and if it's installed later then it will fix up the index.

I note now that 1.1.4 is not available from fedorahosted, and thus I can't fetch the source.  I'm not going to block the review over that as I'm guessing it just hasn't shown up there yet.  There was no problem with 1.1.3 and the sources compared fine.

* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* rpmlint has acceptable complaints.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files
* scriptlets are OK.
* acceptable content.

APPROVED
Comment 20 Nils Philippsen 2008-12-19 05:16:40 EST
(In reply to comment #19)
> Thanks.  Note that we do generally only use needinfo in package reviews as a
> prod in the case one of the parties is not responsive.

I didn't know that, and will try to remember that for future reviews.

> I note now that 1.1.4 is not available from fedorahosted, and thus I can't
> fetch the source.  I'm not going to block the review over that as I'm guessing
> it just hasn't shown up there yet.  There was no problem with 1.1.3 and the
> sources compared fine.

Oops, my bad: I forgot to "make upload". Fixed here and for the other -docs packages.

Thanks for doing the review!
Comment 21 Nils Philippsen 2008-12-19 05:29:34 EST
New Package CVS Request
=======================
Package Name: system-config-services-docs
Short Description: Documentation for configuring system services
Owners: nphilipp
Branches: F-9 F-10
Comment 22 Kevin Fenzi 2008-12-20 23:40:38 EST
cvs done.

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