Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 215200 (pear-Svcs-Weather)
Summary: | Review Request: php-pear-Services-Weather - This class acts as an interface to various online weather-services | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Remi Collet <fedora> |
Component: | Package Review | Assignee: | Christopher Stone <chris.stone> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | Flags: | dennis:
fedora-cvs+
|
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: | 2006-11-22 06:19:14 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: | 214236 | ||
Bug Blocks: | 163779 |
Description
Remi Collet
2006-11-12 10:31:34 UTC
Hi Remi, quick comment before I start the formal review. I looked over your spec file and I wonder why you choose to use # as your delimiter character in your sed command? # is used as a comment in a spec file and this looks very confusing to a reader of the spec file, please change # to @ instead. Also you use %{__sed} however you do not use macros for any other of your system commands, you should be consistent and just use "sed", I think it came up during the discussion of creating the template that macro usage for system commands was only needed in %post(pre)/%post(pre)un sections. Therefore, I would change the sed line to read: sed -i -e s@/usr/local/bin/php@%{_bindir}/php@ $RPM_BUILD_ROOT%{pear_datadir}/%{pear_name}/buildMetarDB.php ==== REVIEW CHECKLIST ==== - rpmlint output clean - package named according to package naming guidelines - spec filename matches %{name} - package meets packaging guidelines - package licensed with open source compatible license - license matches actual license - spec written in American english X spec file is not legible, see comment #1 above. - sed command obfuscated by using comment character (#) as delimeter - source match upstream a83fbf5e2e7ffd22219c513cfefe6b52 Services_Weather-1.4.0.tgz - successfully compiles and builds on FC6 x86_64 - all build dependencies listed in BR - no locales - no shared libraries - not relocatable - owns all directories it creates - no duplicates in %files - file permissions set properly - contains proper %clean section - macro usage consistent - although you use %{__sed} when you dont use macros for any other system command - contains code - no large documentation - files in %doc do not affect run time - no header files or static libraries - no pkgconfig files - no devel subpackage required - no .la files - not a GUI app needing a .desktop file - does not own files or directories owned by other packages ==== MUST ==== - Unobfuscate sed command by using @ instead of # as delimiter (# is used as a comment character in spec files) ==== SHOULD ==== - Change %{__sed} to just "sed" to have consistent macro usage, macros for system commands are only needed for %post/%pre,%postun/%preun sections Hi, thanks for the review. Spec URL: http://remi.collet.free.fr/rpms/extras/php-pear-Services-Weather.spec SRPM URL: http://remi.collet.free.fr/rpms/extras/php-pear-Services-Weather-1.4.0-2.fc7.src.rp %changelog * Sat Nov 18 2006 Remi Collet <Fedora> 1.4.0-2 - Unobfuscate sed command Looks good, APPROVED. Package Change Request ====================== Package Name: php-pear-Services-Weather New Branches: EL-6 Owners: remi CVS Done |