This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 226455 - Merge Review: system-config-date
Merge Review: system-config-date
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: system-config-date (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:04 EST by Nobody's working on this, feel free to take it
Modified: 2009-09-21 16:30 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-04-25 12:43:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:04:49 EST
Fedora Merge Review: system-config-date

http://cvs.fedora.redhat.com/viewcvs/devel/system-config-date/
Initial Owner: nphilipp@redhat.com
Comment 1 Kevin Fenzi 2007-03-20 00:57:38 EDT
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
OK - BuildRequires correct
OK - Spec handles locales/find_lang
See below - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
See below - Package has rm -rf RPM_BUILD_ROOT at top of %install
OK - Package is a GUI app and has a .desktop file
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should have dist tag
OK - Should package latest version
4 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Some of the translation files say:
po/lt.po:# This file is distributed under the same license as the PACKAGE package.
Would be nice to say "system-config-date" there instead of PACKAGE?

2. Since redhat/fedora is upstream for this package, can you add
a note as suggested in:
http://www.fedoraproject.org/wiki/Packaging/SourceURL#head-413e1c297803cfa9de0cc4c56f3ac384bff5dc9e

3. Please use one of the preferred buildroots, such as:
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

4. The desktop file is missing a valid Main Category, see:
http://standards.freedesktop.org/menu-spec/latest/apa.html
Suggest: System or Settings be added.
Without this, this tool shows up under a "Other" menu in Xfce.

5. Why are you manually setting the mode of the man pages and pam files?
Are they not installing with the correct mode?

6. Should add a
rm -rf $RPM_BUILD_ROOT
to the top of the %install section.

7. Are the Obsoletes still needed?
Obsoletes: timetool
Obsoletes: dateconfig
Obsoletes: timeconfig
Obsoletes: redhat-config-date

8. Is the "Conflicts: firstboot <= 1.3.26" needed?
If it still is, couldn't it be converted to a:
"Requires: firstboot => 1.3.26" instead?

9. The "Requires: python2" should probibly just be removed?
The pygtk2-libglade pulls in python.

10. rpmlint says:

a)
W: system-config-date incoherent-version-in-changelog 1.8.90 1.8.90-1.fc7

This is probibly due to missing the version in many of the changelog entries.

b)
E: system-config-date tag-not-utf8 %changelog
E: system-config-date tag-not-utf8 %changelog
E: system-config-date non-utf8-spec-file system-config-date.spec

Suggest: The spec file doesn't seem to be UTF8.
Perhaps run iconv on the spec and check it in again to fix?

c)
E: system-config-date obsolete-not-provided timetool
E: system-config-date obsolete-not-provided dateconfig
E: system-config-date obsolete-not-provided timeconfig
E: system-config-date obsolete-not-provided redhat-config-date
W: system-config-date unversioned-explicit-obsoletes timetool
W: system-config-date unversioned-explicit-obsoletes dateconfig
W: system-config-date unversioned-explicit-obsoletes timeconfig
W: system-config-date unversioned-explicit-obsoletes redhat-config-date

See point 7

d)
E: system-config-date file-in-usr-marked-as-conffile
/usr/share/system-config-date/ntp.template

Suggest: You can't have config marked files in datadir.
Either don't mark it as config, or move it to somewhere else?

e)
E: system-config-date script-without-shebang
/usr/share/system-config-date/timezone_gui.py
E: system-config-date script-without-shebang
/usr/share/system-config-date/dateBackend.py
E: system-config-date script-without-shebang
/usr/share/system-config-date/timezoneBackend.py
E: system-config-date script-without-shebang
/usr/share/system-config-date/scdMainWindow.py
E: system-config-date script-without-shebang
/usr/share/system-config-date/zonetab.py
E: system-config-date script-without-shebang /usr/share/system-config-date/Clock.py
E: system-config-date script-without-shebang
/usr/share/system-config-date/timezone_map_gui.py
E: system-config-date script-without-shebang
/usr/share/system-config-date/system-config-date.glade
E: system-config-date script-without-shebang
/usr/share/system-config-date/date_gui.py

Suggest: All of these should be mode 644 since they are just imported by the
main program? No need for them to be executable.

f)
E: system-config-date no-cleaning-of-buildroot %install

See point 6.

11. 4 outstanding bugs. None of them look to be packaging related, but you
might check them over and see if any can be closed while doing the rest of
the cleanup for this review. 
Comment 2 Nils Philippsen 2007-03-20 04:41:44 EDT
I've changed the following in upstream CVS.

I'll assign this to me and build later when the translations have been updated,
then put it back to be reviewed again.

(In reply to comment #1)
> 1. Some of the translation files say:
> po/lt.po:# This file is distributed under the same license as the PACKAGE package.
> Would be nice to say "system-config-date" there instead of PACKAGE?

Done. Note that there are other mentions of PACKAGE in there, but I'll leave
these to the translators, as they're really only cosmetic.

> 
> 2. Since redhat/fedora is upstream for this package, can you add
> a note as suggested in:
>
http://www.fedoraproject.org/wiki/Packaging/SourceURL#head-413e1c297803cfa9de0cc4c56f3ac384bff5dc9e

Done.

> 3. Please use one of the preferred buildroots, such as:
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Done.

> 4. The desktop file is missing a valid Main Category, see:
> http://standards.freedesktop.org/menu-spec/latest/apa.html
> Suggest: System or Settings be added.
> Without this, this tool shows up under a "Other" menu in Xfce.

Done.

> 5. Why are you manually setting the mode of the man pages and pam files?
> Are they not installing with the correct mode?

Legacy, won't change that now as it might break things (if the Makefile doesn't
set these properly). Might change that when development for F8 opens.

> 6. Should add a
> rm -rf $RPM_BUILD_ROOT
> to the top of the %install section.

Done.

> 7. Are the Obsoletes still needed?
> Obsoletes: timetool
> Obsoletes: dateconfig
> Obsoletes: timeconfig
> Obsoletes: redhat-config-date

They don't hurt and someone might want to update from RHL something to FC7. This
will make a lot of issues for him/her, but at least that package will do the
right thing ;-).

> 8. Is the "Conflicts: firstboot <= 1.3.26" needed?
> If it still is, couldn't it be converted to a:
> "Requires: firstboot => 1.3.26" instead?

No. system-config packages shouldn't require firstboot at all. It is just that
they aren't compatible with older versions, thus the conflict.

> 9. The "Requires: python2" should probibly just be removed?

I've changed that to "python >= 2.0", someone might try to run that on a
python-1.5 based system.

> The pygtk2-libglade pulls in python.

Never trust on some package to pull in another when you're using it yourself ;-).

> 10. rpmlint says:
> 
> a)
> W: system-config-date incoherent-version-in-changelog 1.8.90 1.8.90-1.fc7
> 
> This is probibly due to missing the version in many of the changelog entries.

Probably rather because I use upstream versions (i.e. without release tag) in
there. Won't change that, though ;-).

> b)
> E: system-config-date tag-not-utf8 %changelog
> E: system-config-date tag-not-utf8 %changelog
> E: system-config-date non-utf8-spec-file system-config-date.spec
> 
> Suggest: The spec file doesn't seem to be UTF8.
> Perhaps run iconv on the spec and check it in again to fix?

Ran recode on these files.

> c)
> E: system-config-date obsolete-not-provided timetool
> E: system-config-date obsolete-not-provided dateconfig
> E: system-config-date obsolete-not-provided timeconfig
> E: system-config-date obsolete-not-provided redhat-config-date
> W: system-config-date unversioned-explicit-obsoletes timetool
> W: system-config-date unversioned-explicit-obsoletes dateconfig
> W: system-config-date unversioned-explicit-obsoletes timeconfig
> W: system-config-date unversioned-explicit-obsoletes redhat-config-date
> 
> See point 7
> 
> d)
> E: system-config-date file-in-usr-marked-as-conffile
> /usr/share/system-config-date/ntp.template
> 
> Suggest: You can't have config marked files in datadir.
> Either don't mark it as config, or move it to somewhere else?

Not marked as %config as it really isn't.

> e)
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/timezone_gui.py
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/dateBackend.py
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/timezoneBackend.py
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/scdMainWindow.py
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/zonetab.py
> E: system-config-date script-without-shebang
/usr/share/system-config-date/Clock.py
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/timezone_map_gui.py
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/system-config-date.glade
> E: system-config-date script-without-shebang
> /usr/share/system-config-date/date_gui.py
> 
> Suggest: All of these should be mode 644 since they are just imported by the
> main program? No need for them to be executable.

Not changing modes now as it might break things. Might do later.

> f)
> E: system-config-date no-cleaning-of-buildroot %install
> 
> See point 6.
> 
> 11. 4 outstanding bugs. None of them look to be packaging related, but you
> might check them over and see if any can be closed while doing the rest of
> the cleanup for this review. 

Fixed some this weekend.
Comment 3 Kevin Fenzi 2007-03-20 23:07:36 EDT
Thanks for your quick attention... let me know when you would like me to check
things over again. 

Oh, one additional item... the URL seems to point to a 404 page. 
Is there some better place you could point the URL for this package?
Comment 4 Nils Philippsen 2007-03-22 07:20:41 EDT
I've put up a new s-c-date boilerplate in the Fedora Project wiki and changed
the URL accordingly.

system-config-date-1.8.91-1.fc7 is building right now for Rawhide, it will
possibly take until after test3 is out for the mirrors to have it, but you can
check the spec file directly from CVS if you wish.
Comment 5 Kevin Fenzi 2007-03-23 22:35:26 EDT
Thanks for the excellent attention to all those items...
All look good, except:
 
>> 8. Is the "Conflicts: firstboot <= 1.3.26" needed?
>> If it still is, couldn't it be converted to a:
>> "Requires: firstboot => 1.3.26" instead?
>
>No. system-config packages shouldn't require firstboot at all. It is just that
>they aren't compatible with older versions, thus the conflict.

Conflicts are really nasty. They give the end user an ugly error that they
don't know how to deal with and are generally unhelpful. ;(

http://fedoraproject.org/wiki/PackagingDrafts/Conflicts

When was firstboot 1.3.26 shipped?

>> E: system-config-date script-without-shebang
>> /usr/share/system-config-date/date_gui.py
>>
>> Suggest: All of these should be mode 644 since they are just imported by the
>> main program? No need for them to be executable.
>
>Not changing modes now as it might break things. Might do later.

I can't think of what might break, but I guess it's up to you...

So, no hard blockers, but it would be nice to fix the Conflicts...

any thoughts?
Comment 6 Nils Philippsen 2007-03-26 04:11:12 EDT
Hi Kevin,

the conflicts can't be "fixed": system-config-date fails to work as a plugin for
old firstboot versions. If a conflict gives an "ugly error" to the user, this
error needs to be fixed in yum/pirut/whatever ;-). The old versions of firstboot
were shipped in FC2 and before, so only users upgrading manually (the installer
would resolve that conflict with the new firstboot version) from a version that
old are affected and in this case, they should be presented with it at upgrade
time, not later on with an ugly Python backtrace.

Come to think of it, there have been discussions to "streamline" system-config
tools for a later Fedora version, we might have to revamp the interface to
firstboot again -- in that case, the new system-config tools will conflict with
the up to now current firstboot versions as there is no other way to ensure that
only matching versions of firstboot are installed if (and only if) firstboot is
installed.

On that modes thing, I guess you're right -- if there is breakage people should
notice in the time between now and when we go final. I've built
system-config-date-1.8.92-1.fc7 with these changes.
Comment 7 Nils Philippsen 2007-03-26 06:02:57 EDT
NB: s-c-date doesn't need firstboot and in 1.8.93 I've documented that in the
spec file (according to http://fedoraproject.org/wiki/PackagingDrafts/Conflicts
this is an acceptable use of Conflicts:).
Comment 8 Kevin Fenzi 2007-03-26 15:59:57 EDT
(In reply to comments #6 and #7):

Yeah, I guess that all makes sense. Since it doesn't depend on firstboot... 

I see no further blockers, this package is APPROVED. 
Feel free to close it RAWHIDE once it's been pushed out. 

Thanks for the prompt replies.

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