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

[bugzilla at redhat.com]

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

--- Comment #12 from Renich Bon Ciric <renich at woralelandia.com> ---
(In reply to Björn Esser from comment #9)
> (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.

PHP extensions are never versioned; this is why I ignored this.

> > [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?

The developer insists on this. He's wrote these flags specifically.


> > [-]: 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.

Will do.

> > [-]: 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

Huh? It's not... http://dev1.woralelandia.com/~renich/SPECS/php-phalcon.spec

> > [!]: 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...

Nope, it's there. I think you reviewed an older spec.

> > [ ]: 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?

I'm interested in the checks too. I'll look into it.

>  
> > 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_scanni
> ng

Why do this? Don't these provides help in commands like:

  yum install /usr/lib64/php/modules/phalcon.so
  yum provides "*/phalcon.so*"
?

Anyway, if you have any more references, I'd appreciate it.

> 
> 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}

cool, thanks for the tip ;)
just put that and it will guess the arch's to build?

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

Haven't considered it. Not really acquainted with the reqs...

> A closer look may reveal even more questions.
> 
> 
> Cheers,
>   Björn

Thank you, Björn, for the feedback.

-- 
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=Dj7uqsK8G4&a=cc_unsubscribe