Bug 438811 (php-pear-HTML-AJAX)
Summary: | Review Request: php-pear-HTML-AJAX - PHP and JavaScript AJAX library | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | David Hollis <dhollis> |
Component: | Package Review | Assignee: | Remi Collet <fedora> |
Status: | CLOSED WONTFIX | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | low | ||
Version: | rawhide | CC: | chris.stone, fedora, fedora-package-review, itamar, mtasaka, notting, shamardin, susi.lehtola |
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: | 2009-03-01 08:23:50 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: |
Description
David Hollis
2008-03-25 12:55:01 UTC
Please fix the %files I'm not a sponsor, so this is not a formal review, but I hope this can speed things up a bit. - FAILED: rpmlint complaints: php-pear-HTML-AJAX.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 50) You shoud use spaces for indentation. Just expand all tabs. php-pear-HTML-AJAX.noarch: W: file-not-utf8 /usr/share/doc/php-pear-HTML-AJAX-0.5.2/examples/proxy_usage_server.php php-pear-HTML-AJAX.noarch: W: file-not-utf8 /usr/share/doc/php-pear-HTML-AJAX-0.5.2/examples/support/test.class.php I see that you've tried to fix this issue in the %install script, but you are iconv'ing wrong files. You should iconv files in docdir/%{pear_name}/examples/ - PASSED: The package is named according to the Package Naming Guidelines. - PASSED: Spec file name is ok. - FAILED: The package dos not meet the Packaging Guidelines for PHP. Please check with https://fedoraproject.org/wiki/Packaging/PHP A PEAR package MUST have: BuildRequires: php-pear(PEAR) Requires: php-pear(PEAR) Requires(post): %{__pear} Requires(postun): %{__pear} Provides: php-pear(foo) = %{version} post-scriptlets are fine. - PASSED: The license is fedora-compatible. - FAILED: The license specified on the project homepage is LGPL, the license specified in spec file is LGPLv3. - PASSED: The spec file is written in American English in legible. - PASSED: The sources match the upstream source (md5 verified). - PASSED: The package may be successfully built. - FAILED: Not all build build dependencies are listed in BuildRequires. You should add at least dos2unix. - FAILED: The package does not build in mock due to missing BuildRequires. - PASSED: A package does not contain any duplicate files in the %files listing. - PASSED: Permissions are sane. - PASSED: %clean section is fine. - PASSED: macros usage is fine. - PASSED: The package must contains code. - PASSED: The package does not contain large documentation. - PASSED: %doc contents is fine. - FAILED: This package owns directory /usr/share/pear/HTML. It is already owned by php-pear-HTML-Common, so this package must Require it, and own only it's own subdirectory in /usr/share/pear/HTML. - PASSED: There is rm -rf $RPM_BUILD_ROOT in the beginning of %install. - PASSED: All filenames in rpm package are valid UTF-8. Some other general notes: You are packaging version 0.5.2 of the package which is a beta. There is a newer upstream beta available. Could you please justify why are you packaging an older release, or please upgrade to the newer upstream version. I must disagree with Lev. As this package run without HTML_Common (check all the "require_once" in the scripts), you don't have to "Requires" it. So you must also own the /usr/share/pear/HTML directory. But %file must be cleared Agreed, I forgot to check the requires in the sorce code. BTW, may be this (https://fedoraproject.org/wiki/PackagingDrafts/DirectoryOwnershipImprovement) is already should be included into the guidelines? See http://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership See 2 (Animal example). ++ Then why does this package have to install files under %_datadir/pear/HTML ?? By the way currently on koji-11: # repoquery --repoid=koji-11 --whatprovides '/usr/share/pear/HTML/*' php-pear-HTML-QuickForm-advmultiselect-0:1.4.1-1.fc10.noarch php-pear-HTML-Common-0:1.2.4-2.fc10.noarch php-pear-HTML-QuickForm-0:3.2.10-1.fc9.noarch php-pear-HTML-Table-0:1.8.2-1.fc9.noarch php-pear-HTML-QuickForm-ElementGrid-0:0.1.1-2.fc10.noarch which all Requires php-pear-HTML-Common directly or indirectly. I would recommend removing dos2unix entirely, and just sed which is part of the core build environment. Untested: find docdir -type f -exec sed -i 's/\r//' {} \; In PEAR extension, the tree is a thematic classification (not a dependency one) It's an upstream choice that packager cannot change. Requires must be taken from the PEAR documentation or (best) from the source In QuickForm.php => require_once 'HTML/Common.php'; So php-pear-HTML-QuickForm must requires php-pear-HTML-Common In Table.php => require_once 'HTML/Common.php'; So php-pear-HTML-Table must requires php-pear-HTML-Common In all the script from HTML_AJAX, there is no require/include outside of HTML/AJAX. So no need to require php-pear-HTML-Common ++ No reponse, I close this bug to allow anynody else to submit this package again. |