Bug 253549

Summary: Review Request: TzClock - GTK+ Time Zone Clock
Product: [Fedora] Fedora Reporter: Chris Knight <chris>
Component: Package ReviewAssignee: Rafał Psota <rafalzaq>
Status: CLOSED DUPLICATE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, mtasaka, notting, petersen
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-11-29 05:39:24 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description Chris Knight 2007-08-20 11:11:33 EDT
Spec URL: http://www.theknight.co.uk/releases/tzclock.spec
SRPM URL: http://www.theknight.co.uk/releases/tzclock-2.51-1.src.rpm
Description: While working aboard I looked for a nice graphical clock that could display the time for many locations.  Because I could not find one that did all that I needed I developed TzClock.
Comment 1 Jens Petersen 2007-09-05 04:23:17 EDT
A few comment:
- How about a desktop file?
- Would be nice to use %configure and %{_bindir}.
- changelog entry would be nice too
- you should not use the Packager field in fedora packages: your name will be in
the changelog
- url to source tarball should be used
- I would replace "X" by "Cairo" say in the Summary.

You probably want to run rpmlint on the srpm and rpm to check for more things.

Very cool clock! :-)
Comment 2 Chris Knight 2007-09-05 09:59:17 EDT
(In reply to comment #1)
> A few comment:
> - How about a desktop file?
> - Would be nice to use %configure and %{_bindir}.
> - changelog entry would be nice too
> - you should not use the Packager field in fedora packages: your name will be in
> the changelog
> - url to source tarball should be used
> - I would replace "X" by "Cairo" say in the Summary.
> 
> You probably want to run rpmlint on the srpm and rpm to check for more things.
> 
> Very cool clock! :-)

Thank you for your comments, I am still learning about building rpms so all help
is welcome.  I have updated the spec file, and uploaded a new version.

I am interested in your question about a desktop file.  Where can I find out
more about these?

Thanks again, Chris.
Comment 3 Rafał Psota 2007-09-05 10:15:18 EDT
Guidelines are very useful. :)
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755
Comment 4 Chris Knight 2007-09-05 14:52:17 EDT
Greak link, the new version with a .desktop file has been uploaded.

http://www.theknight.co.uk/releases/tzclock-2.5.3-3.src.rpm

Thanks for your help,  Chris
Comment 5 Jens Petersen 2007-09-06 01:29:36 EDT
Thanks for the updates - looks better now. :)

I would also suggest now removing the superfluous definitions for 
%name, %version and %release at the top - you will get those for free
from the Name, Version and Release fields. :)
Comment 6 Chris Knight 2007-09-06 05:54:27 EDT
Fixed it :-)  It was taken from an example on the Mandriva site, I should not
trust them ;-)
Comment 7 Rafał Psota 2007-09-28 09:40:56 EDT
Above link is broken so I use this one:
http://www.theknight.co.uk/releases/tzclock-2.5.4-1.src.rpm

* rpmlint is not silent:
tzclock-debuginfo.x86_64: E: empty-debuginfo-package
tzclock.src: E: invalid-spec-name TzClock.spec
* you should use %{?dist} macro in Release tag
* package fails to build in mock; missing BR: libgnomeui-devel
* you should include AUTHORS and COPYING files in %doc
* IMO you should change all names to lowercase to improve usability
* IANAL but this doesn't look like GPLv2
/*----------------------------------------------------------------------------------------------------*
 *                                                                             
                      *
 *  T Z  C L O C K  G T K . C                                                  
                      *
 *  =========================                                                  
                      *
 *                                                                             
                      *
 *  All rights reserved. Reproduction, modification, use or                    
                      *
 *  disclosure to third parties without express authority is                   
                      *
 *  forbidden. Copyright Teligent AB, Sweden, 2006.                            
                      *
 *                                                                             
                      *
 *----------------------------------------------------------------------------------------------------*/

Comment 8 Chris Knight 2007-09-29 16:55:26 EDT
(In reply to comment #7)
> * rpmlint is not silent:
> tzclock-debuginfo.x86_64: E: empty-debuginfo-package
debuginfo package removed.
> tzclock.src: E: invalid-spec-name TzClock.spec
spec file reverted to lowercase.
> * you should use %{?dist} macro in Release tag
Done, always wondered how it should be done.
> * package fails to build in mock; missing BR: libgnomeui-devel
Added build requirement, all this for a link on the about box.
> * you should include AUTHORS and COPYING files in %doc
Done.
> * IMO you should change all names to lowercase to improve usability
spec file at least is lowercase.
> * IANAL but this doesn't look like GPLv2
Oooops my auto comment program caught me out.

Thanks for your feedback, it is very helpful.
Comment 9 Jason Tibbitts 2007-09-29 17:51:53 EDT
BTW, simply disabling the debuginfo package isn't usually the right thing to do.
 You need to understand why it's ending up empty.  Either the package has no
binaries, the proper CFLAGS aren't getting passed to the compiler, or something
is stripping the binaries.  All of these indicate some problem with the package.
Comment 10 Ralf Corsepius 2007-09-30 03:04:25 EDT
(In reply to comment #9)
> the proper CFLAGS aren't getting passed to the compiler,

This applies here. The Makefile.am is broken. It doesn't apply *_CFLAGS correctly.




Comment 11 Mamoru TASAKA 2007-09-30 04:21:26 EDT
(In reply to comment #9)
> BTW, simply disabling the debuginfo package isn't usually the right thing to do.
>  You need to understand why it's ending up empty.  Either the package has no
> binaries, the proper CFLAGS aren't getting passed to the compiler, or something
> is stripping the binaries.  All of these indicate some problem with the package.

(In reply to comment #10)
> (In reply to comment #9)
> > the proper CFLAGS aren't getting passed to the compiler,
> 
> This applies here. The Makefile.am is broken. It doesn't apply *_CFLAGS correctly.
> 

Well I tried to rebuild 2.5.4-2 (a bit modification needed
for meaningless line "%debug_package %{nil}")
http://koji.fedoraproject.org/koji/taskinfo?taskID=178885

It seems that fedora compilation flags are correctly honored
http://koji.fedoraproject.org/koji/getfile?taskID=178885&name=build.log

But the line:
------------------------------------------------------------
+ install -s -m 755 TzClockCairo
/var/tmp/tzclock-2.5.4-2.2.fc8-buildroot/usr/bin/TzClock
------------------------------------------------------------
is actually problematic.
Comment 12 Mamoru TASAKA 2007-09-30 04:26:08 EDT
One more note:

When I tried to remove "-s" option from "install", the result is
http://koji.fedoraproject.org/koji/taskinfo?taskID=178880
http://koji.fedoraproject.org/koji/getfile?taskID=178880&name=build.log

-----------------------------------------------------
+ /usr/lib/rpm/find-debuginfo.sh /builddir/build/BUILD/tzclock-2.5.4
extracting debug info from /var/tmp/tzclock-2.5.4-2.1.fc8-buildroot/usr/bin/TzClock
260 blocks
-----------------------------------------------------
Comment 13 Chris Knight 2007-09-30 05:42:43 EDT
(In reply to comment #12)
I have removed the -s from the latest tzclock.spec and put the debuginfo back in:
http://www.tzclock.org/releases/tzclock.spec

Thanks again for all you help.  I hope to right this up one day so others can
learn from my mistakes.  
Comment 14 Ralf Corsepius 2007-09-30 12:04:39 EDT
(In reply to comment #11)
> (In reply to comment #9)

> (In reply to comment #10)
> > (In reply to comment #9)
> > > the proper CFLAGS aren't getting passed to the compiler,
> > 
> > This applies here. The Makefile.am is broken. It doesn't apply *_CFLAGS
correctly.
> > 
>
> It seems that fedora compilation flags are correctly honored
> http://koji.fedoraproject.org/koji/getfile?taskID=178885&name=build.log
Not for me.

This Makefile.am is broken.

It uses
*_CFLAGS = -DXXX
this overrides automake's internal CFLAGS and violates automake working-principles.

Correct usage would be:
*_CFLAGS = $(AM_CFLAGS) <more-flags>

Further abuse: -DXXX are CPPFLAGS.
They do not belong into CFLAGS, they'd belong into *_CPPFLAGS

For detail consult the fine manual (info Automake).

Comment 15 Chris Knight 2007-09-30 13:37:04 EDT
(In reply to comment #14)
From reading the manual and going on what you are saying the mistake is using
*_CFLAGS at all.  Because -D is a pre-processor command it should be included in
the *_CPPFLAGS.  The flags can be modified without causing problems so...

TzClockCairo_CPPFLAGS = -DBUILDCAIRO

...is OK and follows the automake rules.
Comment 16 Ralf Corsepius 2007-09-30 23:43:39 EDT
(In reply to comment #15)
> (In reply to comment #14)
> From reading the manual and going on what you are saying the mistake is using
> *_CFLAGS at all. 
Not quite. There are 2 small bugs interacting at the same time.

1) By using *_CFLAGS, you are _overriding_ automake's default CFLAGS's behavior.
What you intend to do is extending CFLAGS. To achieve this you need to add
$(AM_CFLAGS) to *_CFLAGS.

2) Passing preprocessor flags though *CFLAGS.
Autoconf and automake distinguish preprocessor flags from "<language>-FLAGS".
(CFLAGS, CXXFLAGS etc.)

> Because -D is a pre-processor command it should be included in
> the *_CPPFLAGS. 
Right.

> The flags can be modified without causing problems so...
> 
> TzClockCairo_CPPFLAGS = -DBUILDCAIRO
> 
> ...is OK and follows the automake rules.
Right. In particular, it avoids interferring with CFLAGS ;)
Comment 17 Chris Knight 2007-10-01 11:24:32 EDT
(In reply to comment #16)

I have updated the sources:

http://theknight.co.uk/releases/tzclock-2.5.4-4.src.rpm

This is a good learning experience! :-)
Comment 18 Mamoru TASAKA 2007-10-01 12:54:04 EDT
Chris,

It seems you are not in cvsextras member and this is the
first review request you submitted. Do you have any sponsor?
Comment 19 Chris Knight 2007-10-01 13:38:09 EDT
(In reply to comment #18)

Hello, you are right this is my first review request and I don't have a sponsor.
 I have been a commercial coder for 20+ years working on nearly all the
different PC OS's.  I am currently developing telecommunications applications
than run on Redhat EL.  Hence my interest in Fedora and giving something back to
the community.

Comment 20 Rafał Psota 2007-10-01 14:02:38 EDT
In this case I can't approve this package because I'm not a sponsor.
Comment 21 Chris Knight 2007-10-03 10:27:47 EDT
As this route is proving hard I have added it to GnomeFiles:

http://www.gnomefiles.org/app.php/TzClock
Comment 22 Jens Petersen 2007-10-04 01:58:19 EDT
Chris, thanks for your patience.

Please see <http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored>.
Comment 23 Mamoru TASAKA 2007-10-09 12:25:59 EDT
Well, as no one is reviewing:

For 2.5.4-4:
* Timestamp
  - Please keep timestamps on installed files when possible
    (check the section "Timestamps" of
     http://fedoraproject.org/wiki/Packaging/Guidelines )

    In short, when using "cp" or "install" command, please
    use "-p" option.

* desktop-file-install vendor
  - We still recommend "fedora" vendor prefix
    ("desktop-file-install usage" section of the URL above).

* macro
  - %_datadir/man should be %_mandir

* %defattr
  - We now recommend %defattr(-,root,root,-)

Then:
Comment 24 Mamoru TASAKA 2007-10-09 12:27:41 EDT
(In reply to comment #23)
> Then:
(sorry, my comments follow:)
Then:
-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 25 Chris Knight 2007-10-09 14:52:05 EDT
(In reply to comment #23)

> * desktop-file-install vendor
>   - We still recommend "fedora" vendor prefix
>     ("desktop-file-install usage" section of the URL above).

Is there a macro that could be used to get the vendor (with a value of fedora)?
 I would like to have only one spec file for all distributions.  I guess the
other option is to add it to configure.
Comment 26 Mamoru TASAKA 2007-10-09 22:09:19 EDT
(In reply to comment #25)
> (In reply to comment #23)
> 
> > * desktop-file-install vendor
> >   - We still recommend "fedora" vendor prefix
> >     ("desktop-file-install usage" section of the URL above).
> 
> Is there a macro that could be used to get the vendor (with a value of fedora)?
I guess there is not.

> I would like to have only one spec file for all distributions. 
Please don't stick to this idea.

One more comment
* %?dist tag in %changelog
  - Remove %?dist tag from %changelog. %?dist tag changes when
    Fedora version changes, which means that %changelog is
    changed when Fedora version changes.
Comment 27 Mamoru TASAKA 2007-10-15 11:22:50 EDT
ping?
Comment 28 Chris Knight 2007-10-15 16:13:42 EDT
(In reply to comment #26)
Sorry for the slow reply.  I did find a vendor in one of the macros but it was
set to Readhat.  Maybe one day all the distro's could get together to come up
with standard macros so that a common spec file would be possible.

Anyway I have uploaded a new spec file:

http://tzclock.org/releases/tzclock.spec

There is still one dist in the change log to stop a warning from rpmlint.  Other
wise all your suggestions are in there.

Thank you for looking,  Chris
Comment 29 Mamoru TASAKA 2007-10-16 12:07:53 EDT
Well,
! From next time, please make srpm and post the URL for the srpm.

* %{?dist} tag in the last %changelog entry is actually not needed.
  When you rebuild your srpm by mock or on koji, the rpmlint should
  disappear.

So as tzclock package itself is okay, I will wait for your pre-review
or another review request as I said in comment 24.
Comment 30 Mamoru TASAKA 2007-11-01 12:37:59 EDT
ping again?
Comment 31 Mamoru TASAKA 2007-11-17 08:25:40 EST
Again ping?

It may be that this bug will be closed if no response from the reporter
is received within ONE WEEK.
Comment 32 Mamoru TASAKA 2007-11-29 05:39:24 EST
Once closing.

If someone wants to package this software and import it into Fedora,
please file another review request, thanks.
Comment 33 Mamoru TASAKA 2007-12-05 07:07:23 EST

*** This bug has been marked as a duplicate of 411591 ***