[Bug 977208] Review Request: Phalcon - A web framework implemented as a C extension

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=977208

Björn Esser <bjoern.esser@xxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bjoern.esser@xxxxxxxxx

--- Comment #9 from Björn Esser <bjoern.esser@xxxxxxxxx> ---
(In reply to Roman Mohr from comment #7)

Nice review, Roman.  But here are some things / questions I stumbled upon just
looking on spec-file...


> [-]: Development (unversioned) .so files in -devel subpackage, if present.
>      Note: Unversioned so-files in private %_libdir subdirectory (see
>      attachment). Verify they are not in ld path.

This is usually intentional on c-compiled language-plugins, since they are
loaded with dlopen on inclusion.


> [x]: %build honors applicable compiler flags or justifies otherwise.

Should be FAIL, because spec-file appends flags to system-flags:
CFLAGS="%{optflags} -O2 -fno-delete-null-pointer-checks -finline-functions
-fomit-frame-pointer"  Why?  What's the use/benefit of that?  Any explanation?


> [-]: Each %files section contains %defattr if rpm < 4.4
>      Note: %defattr present but not needed

%defattr is obsolete and needed for rpm < 4.4, only.  Even el5 shippes a more
recent version.  Please remove.


> [-]: Package is not known to require ExcludeArch.

Why does the spec have ExclusiveArch:  x86_64 i686?  Is there any special
reason for it?  Are there really build errors?  Can you provide a build.log
from any failing arch?

If there's a good reason for it, then at least follow the process described in
the packaging-guidelines:
https://fedoraproject.org/wiki/Packaging/Guidelines#Architecture_Support


> [x]: If the package is under multiple licenses, the licensing breakdown must
>      be documented in the spec.

MIT is missing in License-Tag, so it should be FAIL here


> [!]: If the source package does not include license text(s) as a separate
> file from upstream, the packager SHOULD query upstream to include it.

License.{md,txt} is present somewhere in tarball, but not picked on %doc...


> [ ]: Final provides and requires are sane (see attachments).

You should filter the private so-object from provides, see below.


> [-]: %check is present and all tests pass.
> 
>      ---> There are a lot of unit tests but they are only applicable if
>           compiled from the development branch. No blocker.

Is there any way to run the checks during build, e.g. initializing a fake git
repo in source-tree or some other hack?


> Rpmlint (installed packages)
> ----------------------------
> # rpmlint php-phalcon
> php-phalcon.x86_64: W: private-shared-object-provides
> /usr/lib64/php/modules/phalcon.so phalcon.so()(64bit)
> php-phalcon.x86_64: W: no-documentation
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.
> # echo 'rpmlint-done:'
>
> 
> Provides
> --------
> php-phalcon:
>     config(php-phalcon)
>     phalcon.so()(64bit)
>     php-phalcon
>     php-phalcon(x86-64)

You should filter the private so provides. See instructions here:
https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Filtering_provides_and_requires_after_scanning


Why are you using such a construct during %prep?

 | %ifarch x86_64
 |   %global builddir "build/64bits"
 | %endif
 |
 | %ifarch i686
 |   %global builddir "build/32bits"
 | %endif

You can archive this really simpler by just initializing the macro on top of
spec-file using:

   %global builddir build-%{?_arch}


Why don't you want to build this for el5/el6?


A closer look may reveal even more questions.


Cheers,
  Björn

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=2Gh3vBI0AJ&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]