Bug 641188

Summary: Review Request: libgweather3 - A library for weather information
Product: [Fedora] Fedora Reporter: Matthias Clasen <mclasen>
Component: Package ReviewAssignee: Parag AN(पराग) <panemade>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: bnocera, fedora-package-review, notting, panemade
Target Milestone: ---Flags: panemade: 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: 2010-11-25 14:28:46 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: 201449    
Attachments:
Description Flags
spec cleanup none

Description Matthias Clasen 2010-10-08 00:45:02 UTC
libgweather is a library to access weather information from online
services for numerous locations. libgweather3 is a version of
libgweather for use with GTK+ 3.

Spec: http://mclasen.fedorapeople.org/libgweather3.spec
Srpm: http://mclasen.fedorapeople.org/libgweather3-2.91.0-1.fc15.src.rpm

When this is approved, I will change libgweather to also depend on libgweather-data (needed to avoid file conflict between the packages).

Comment 1 Parag AN(पराग) 2010-10-12 07:33:02 UTC
Suggestions:-

1) I see this package includes versioned BuildRequires. Can you add some comments why is it needed to have versioned BR: ? 
   I think as we used to first build required build dependencies in required release, there should not be any need to write versions.

2) For usage of obsoletes and provides see http://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages#Do_I_need_to_Provide_my_old_package_names.3F

this package added
Obsoletes:      gnome-applets-devel < 1:2.21.4-1

but I see gnome-applets-devel-2.21.4-1 was built for FC9. So, this obsolete is not needed.

3) Is there any bug reference for Patch0: ?

4) Please we have got updated Gconf2 scriptlet snippet. Good to use that. See 

5) Any reason to turn off verbose output in %posttrans?

What does gtkdocize do?

Comment 2 Parag AN(पराग) 2010-10-12 07:48:44 UTC
for GConf scriptlet snippet, https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf

Comment 3 Parag AN(पराग) 2010-10-12 07:52:17 UTC
Created attachment 452894 [details]
spec cleanup

Comment 4 Matthias Clasen 2010-10-12 17:15:22 UTC
(In reply to comment #1)
> Suggestions:-
> 
> 1) I see this package includes versioned BuildRequires. Can you add some
> comments why is it needed to have versioned BR: ? 
>    I think as we used to first build required build dependencies in required
> release, there should not be any need to write versions.

At least the gtk3 version is somewhat useful, as the package won't build against 2.90.x


> this package added
> Obsoletes:      gnome-applets-devel < 1:2.21.4-1

Huh ? I see no Obsoletes in the spec.
 

> 3) Is there any bug reference for Patch0: ?

No, its the same patch that is in the libgweather package


> 4) Please we have got updated Gconf2 scriptlet snippet. Good to use that. See 

True


> 5) Any reason to turn off verbose output in %posttrans?

People complain if scriptlets are verbose

Comment 5 Parag AN(पराग) 2010-10-13 00:43:35 UTC
(In reply to comment #4)
> (In reply to comment #1)
> > Suggestions:-
> > 
> > 1) I see this package includes versioned BuildRequires. Can you add some
> > comments why is it needed to have versioned BR: ? 
> >    I think as we used to first build required build dependencies in required
> > release, there should not be any need to write versions.
> 
> At least the gtk3 version is somewhat useful, as the package won't build
> against 2.90.x
> 

Ok. If you need it you can keep it. But please remove others.

> 
> > this package added
> > Obsoletes:      gnome-applets-devel < 1:2.21.4-1
> 
> Huh ? I see no Obsoletes in the spec.

There it happens mismatch. Sometimes it happens srpm contains some other spec and SPEC url shows some other spec contents. I can still see obsoletes in spec packaged in srpm.
 I see no issues then as it looks spec url shows no obsoletes.
 
> 
> > 3) Is there any bug reference for Patch0: ?
> 
> No, its the same patch that is in the libgweather package

 Ok, but good if some bug reference would have given for that patch.

 
> 
> > 4) Please we have got updated Gconf2 scriptlet snippet. Good to use that. See 
> 
> True
> 
> 
> > 5) Any reason to turn off verbose output in %posttrans?
> 
> People complain if scriptlets are verbose

But good to make it verbose so that people can see if anything goes wrong in executing that scriptlet. If you still think -q should be used then no problem use it.

Comment 6 Matthias Clasen 2010-10-15 15:55:40 UTC
Spec: http://mclasen.fedorapeople.org/libgweather3.spec
Srpm: http://mclasen.fedorapeople.org/libgweather3-2.91.0-2.fc15.src.rpm

I've dropped most library versions, and the -q from %posttrans

Comment 7 Parag AN(पराग) 2010-10-15 17:04:27 UTC
Please can you tell me any reason for using old Gconf2 scriptlet snippet?

Also, add missing
Requires(pre): GConf2
Requires(post): GConf2
Requires(preun): GConf2


I can review and approve this now but unfortunately, there are many packages still using old GConf2 scriptlets and now this new package is also using old snippet.

Comment 8 Matthias Clasen 2010-10-15 17:16:52 UTC
No particular reason, just forgot. I can fix it during the import, if thats good enough for you.

Comment 9 Parag AN(पराग) 2010-10-18 04:30:58 UTC
Suggestions:
1) Add BR: gobject-introspection-devel

2) Add following to -devel
Requires:       %{name} = %{version}-%{release}

3) Add following to libgweather-data
%defattr(-,root,root,-)

4) Use the correct GConf2 scriptlets

5) Good to preserve timestamps in make install command

6) Is there any reason not to create libgweather3-data? Good to add some comment in spec to have subpackage name as libgweather-data

APPROVED.

Comment 10 Matthias Clasen 2010-10-18 13:15:45 UTC
> 6) Is there any reason not to create libgweather3-data? Good to add some
> comment in spec to have subpackage name as libgweather-data

The only reason I did it this way is that it would be a little odd if libgweather depended on libgweather3-data.

Thanks for approving. I'm out of the country this week, will get to importing this (with the requested fixes) when I'm back next week.

Comment 11 Parag AN(पराग) 2010-11-12 01:52:23 UTC
ping

Comment 12 Parag AN(पराग) 2010-11-18 06:17:58 UTC
One month passed since this review is approved. time to needinfo on reporter.