Bug 641188 - Review Request: libgweather3 - A library for weather information
Review Request: libgweather3 - A library for weather information
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2010-10-07 20:45 EDT by Matthias Clasen
Modified: 2010-11-25 09:28 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-11-25 09:28:46 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
panemade: fedora‑review+


Attachments (Terms of Use)
spec cleanup (2.78 KB, patch)
2010-10-12 03:52 EDT, Parag AN(पराग)
no flags Details | Diff

  None (edit)
Description Matthias Clasen 2010-10-07 20:45:02 EDT
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 03:33:02 EDT
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 03:48:44 EDT
for GConf scriptlet snippet, https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf
Comment 3 Parag AN(पराग) 2010-10-12 03:52:17 EDT
Created attachment 452894 [details]
spec cleanup
Comment 4 Matthias Clasen 2010-10-12 13:15:22 EDT
(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-12 20:43:35 EDT
(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 11:55:40 EDT
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 13:04:27 EDT
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 13:16:52 EDT
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 00:30:58 EDT
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 09:15:45 EDT
> 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-11 20:52:23 EST
ping
Comment 12 Parag AN(पराग) 2010-11-18 01:17:58 EST
One month passed since this review is approved. time to needinfo on reporter.

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