Bug 641188
Summary: | Review Request: libgweather3 - A library for weather information | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Matthias Clasen <mclasen> | ||||
Component: | Package Review | Assignee: | Parag AN(पराग) <panemade> | ||||
Status: | CLOSED NOTABUG | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | 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
Matthias Clasen
2010-10-08 00:45: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? for GConf scriptlet snippet, https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf Created attachment 452894 [details]
spec cleanup
(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 (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. 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 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. No particular reason, just forgot. I can fix it during the import, if thats good enough for you. 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. > 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.
ping One month passed since this review is approved. time to needinfo on reporter. |