Bug 977208

Created attachment 766491 [details]
uses unmodified upstream tarball

I can't do a proper review at the moment, because I am not jet familiar with the php packaging guidlines, I first have to study them more closely. But I have notices that you modified to included source tarball. With this patch you can use the unmodified upstream tarball.

(In reply to Roman Mohr from comment #1)
> Created attachment 766491 [details]
> uses unmodified upstream tarball
> 
> I can't do a proper review at the moment, because I am not jet familiar with
> the php packaging guidlines, I first have to study them more closely. But I
> have notices that you modified to included source tarball. With this patch
> you can use the unmodified upstream tarball.

Thanks dude. 

I didn't patch the tar. I did a git clone and then git archive. This should've omitted some .gitignore and other files.

I tried building and it didn't work... did you test it?

So, a minor update. It works now with the package downloaded directly from github; no git archive or anything...

SPEC: http://dev1.woralelandia.com/~renich/SPECS/php-phalcon.spec
SRPM: http://dev1.woralelandia.com/~renich/SRPMS/php-phalcon-1.2.0-2.fc18.src.rpm

It is better to name the tar.gz like %{name}-%{version}.tar.gz as in the patch ( see point "Source0" in http://fedoraproject.org/wiki/How_to_create_an_RPM_package#SPEC_file_overview). Otherwise your tar.gz might override other tar.gz's in your SOURCES directory which are also not properly named.

The patch works for me. Did you place the source tar.gz in your ~/rpmbuild/SOURCES directory?

After patching the spec file I tested it with something like this:

spectool -g php-phalcon.spec
mock -r fedora-rawhide-x86_64 --buildsrpm --spec=php-phalcon.spec --sources=. --resultdir=.
mock -r fedora-rawhide-x86_64 --no-clean --no-cleanup-after --resultdir=. php-phalcon-1.2.0-1.fc20.src.rpm

Che

(In reply to Roman Mohr from comment #4)
> It is better to name the tar.gz like %{name}-%{version}.tar.gz as in the
> patch ( see point "Source0" in
> http://fedoraproject.org/wiki/
> How_to_create_an_RPM_package#SPEC_file_overview). Otherwise your tar.gz
> might override other tar.gz's in your SOURCES directory which are also not
> properly named.
> 
> The patch works for me. Did you place the source tar.gz in your
> ~/rpmbuild/SOURCES directory?

Yes; I did. The thing is that the package should be named php-phalcon for 2 reasons:

- Fedora frameworks and pecl libs are named php-<name>
- The package is named cphalcon; but the project is called Phalcon. Believe me; I asked the author about it.

So, your script works fine with cphalcon as a name. Not with php-phalcon as a name.

> After patching the spec file I tested it with something like this:
> 
> spectool -g php-phalcon.spec
> mock -r fedora-rawhide-x86_64 --buildsrpm --spec=php-phalcon.spec
> --sources=. --resultdir=.
> mock -r fedora-rawhide-x86_64 --no-clean --no-cleanup-after --resultdir=.
> php-phalcon-1.2.0-1.fc20.src.rpm

I could've patched it wrongly. I did patch orig file.patch

Thank you for your interest! ;)

(In reply to Renich Bon Ciric from comment #5)
> (In reply to Roman Mohr from comment #4)
> > It is better to name the tar.gz like %{name}-%{version}.tar.gz as in the
> > patch ( see point "Source0" in
> > http://fedoraproject.org/wiki/
> > How_to_create_an_RPM_package#SPEC_file_overview). Otherwise your tar.gz
> > might override other tar.gz's in your SOURCES directory which are also not
> > properly named.
> > 
> > The patch works for me. Did you place the source tar.gz in your
> > ~/rpmbuild/SOURCES directory?
> 
> Yes; I did. The thing is that the package should be named php-phalcon for 2
> reasons:
> 
> - Fedora frameworks and pecl libs are named php-<name>
> - The package is named cphalcon; but the project is called Phalcon. Believe
> me; I asked the author about it.

That's ok, but that is not what I meant. I just said that I think it is a good idea to name the source tarball like the rpm package.

> So, your script works fine with cphalcon as a name. Not with php-phalcon as
> a name.

Well that is not correct. If you download the tarball (after patching the spec file) via "spectool -g php-phalcon.spec" you will get a source file called php-phalcon-1.2.0.tar.gz (see https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Using_.25.7Bversion.7D):

----> Source0:        https://github.com/%{_name}/c%{_name}/archive/%{version}.tar.gz#/%{name}-%{version}.tar.gz

So after patching, download the source tarball via spectool and put it into rpmbuild/SOURCES or rename your sources there to "php-phalcon-1.2.0.tar.gz".

But still, when you extract it, the extracted folder is called "cphalcon-1.2.0" and this is why I apply this to %%setup:

----> %setup -q -n c%{_name}-%{version}


> > After patching the spec file I tested it with something like this:
> > 
> > spectool -g php-phalcon.spec
> > mock -r fedora-rawhide-x86_64 --buildsrpm --spec=php-phalcon.spec
> > --sources=. --resultdir=.
> > mock -r fedora-rawhide-x86_64 --no-clean --no-cleanup-after --resultdir=.
> > php-phalcon-1.2.0-1.fc20.src.rpm
> 
> I could've patched it wrongly. I did patch orig file.patch
> 
> Thank you for your interest! ;)

I hope I will find some time tomorrow ( I cant promise) for a proper review.

Cheers,

Roman

Created attachment 767176 [details]
improved spec file

There are some smaller Issues, see attached report. Most of them are fixed in the attachment.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[-]: 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.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[!]: Changelog in prescribed format.

     ---> Your name is missing in the changelog headlines.

[x]: Sources contain only permissible code or content.
[-]: Each %files section contains %defattr if rpm < 4.4
     Note: %defattr present but not needed
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[-]: Package is not known to require ExcludeArch.
[!]: Package complies to the Packaging Guidelines

     ---> Source0 references the head of a git branch and not a tag, so please
          add a stable link to a commit instead of a link to HEAD. See the 
          attached patch for a solution. Thats also the reason why the 
          checksums of the sources do not match.

[!]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.

     ---> I would suggest to add this to %files:
          %doc CHANGELOG CONTRIBUTING.md README.md docs/DOCUMENTATION.txt  docs/LICENSE.md  docs/LICENSE.txt

[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (3 clause)", "MIT/X11 (BSD like)", "Unknown or generated". 901 files
     have unknown license. Detailed output of licensecheck in
     /home/roman/rpmbuild/REVIEWS/php-phalcon/licensecheck.txt

     ---> MIT license is missing

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[-]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[!]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[ ]: Final provides and requires are sane (see attachments).
[?]: Package functions as described.

     ---> next on my list
 
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %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.

[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: php-phalcon-1.2.0-2.fc18.x86_64.rpm
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.




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:'



Requires
--------
php-phalcon (rpmlib, GLIBC filtered):
    config(php-phalcon)
    libc.so.6()(64bit)
    php(api)
    php(zend-abi)
    rtld(GNU_HASH)



Provides
--------
php-phalcon:
    config(php-phalcon)
    phalcon.so()(64bit)
    php-phalcon
    php-phalcon(x86-64)



Unversioned so-files
--------------------
php-phalcon: /usr/lib64/php/modules/phalcon.so

Source checksums
----------------
https://github.com/phalcon/cphalcon/archive/1.2.0.tar.gz :
  CHECKSUM(SHA256) this package     : 7317a7bdea1b372c4968a8c4f258ff126b9d3e73d8f962c35eaec54957cb048f
  CHECKSUM(SHA256) upstream package : a2adcbb47fb8d76108e0a870241aaec068193a790badc22eb1a9588f53e798d1
diff -r also reports differences

Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-18-x86_64
Command line :/usr/bin/fedora-review -rn php-phalcon

Ok, updated SPEC and SRPM

SPEC: http://dev1.woralelandia.com/~renich/SPECS/php-phalcon.spec
SRPM: http://dev1.woralelandia.com/~renich/SRPMS/php-phalcon-1.2.0-3.4b72745.fc18.src.rpm

(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

Created attachment 767312 [details]
improvements for spec-file

Here is a patch which solves some the problems I mentioned, especially the ExcludeArch issue.

The other attachment is needed additionally.

Created attachment 767313 [details]
use install-script to get some needed parameters for build

This is needed in addition to the spec-file improvements.

(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.

(In reply to Björn Esser from comment #11)
> Created attachment 767313 [details]
> use install-script to get some needed parameters for build
> 
> This is needed in addition to the spec-file improvements.

I don't understand. That script is just determining if the build is a 64bit or 32bit. It compiles a program in order to determine this.

I don't see the need for it. This is taken care of in the if clauses on the SPEC file, is it not?

Created attachment 771888 [details]
Patch to run most of the non-db-dependent unit tests

When adding 'php-mcrypt' and 'php-phpunit-PHPUnit' as build-dependencies this patch makes most of the non-db-dependent unit-tests run.

(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...

Thx for your detailed comments Björn.

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

You are right deeper inspection is necessary.
 
> 
> > [-]: %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?

See the patch above :) . One Question remains: Is there a standard way (or a way at all) to run unit-tests which depend on a database? Is it possible to just install and start a server in koji? I have to check/try that.

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

Thx for the link.

Created attachment 771892 [details]
using the new v1.2.0 github tag

Btw. a git tag for version 1.2.0 seems to be available now.

(In reply to Björn Esser from comment #11)
> Created attachment 767313 [details]
> use install-script to get some needed parameters for build
> 
> This is needed in addition to the spec-file improvements.

Hello,

IMHO, this is totally unnecessary and overcomplicated. That binary decides if this is a 64bit or 32bit system; in an overcomplicated way, if you ask me.

I'd rather implement this on the SPEC file and not rely on some C script to do it; it's redundant. What do you think?

Here's the code for gccarch.c; which is obvious.

## gccarch.c 
#include <stdio.h>

// Check windows
#if _WIN32 || _WIN64
#if _WIN64
#define ENVIRONMENT64 
#else
#define ENVIRONMENT32
#endif
#endif

// Check GCC
#if __GNUC__
#if __x86_64__ || __ppc64__
#define ENVIRONMENT64
#else
#define ENVIRONMENT32
#endif
#endif

int main(){
#ifndef ENVIRONMENT32
printf("%d", 1);
#else
printf("%d", 0);
#endif
}

Here's the SRPM and SPEC files with almost all fixes. Still need to implement the test fix.

SPEC: http://dev1.woralelandia.com/~renich/SPECS/php-phalcon.spec
SRPM: http://dev1.woralelandia.com/~renich/SRPMS/php-phalcon-1.2.0-6.fc19.src.rpm

Created attachment 773907 [details]
patch for spec file to run unittests

patches the spec file to sucessfully run phpunit

Created attachment 773914 [details]
Patch to run most of the non-db-dependent unit tests

Apply in combination with attachment 773907 [details]

(In reply to Roman Mohr from comment #20)
> Created attachment 773914 [details]
> Patch to run most of the non-db-dependent unit tests
> 
> Apply in combination with attachment 773907 [details]

Thank you, Roman, for your hard work and collaboration.

The thing is that I may be formating the patch wrongly. Here's what I did:

- manually changed the SPEC file
- got the contents of your patch and put them in a file called php-phalcon-phpunit.patch

Here's what I got:

Patch #0 (php-phalcon-phpunit.patch):
+ /usr/bin/cat /builddir/build/SOURCES/php-phalcon-phpunit.patch
+ /usr/bin/patch -p0 --fuzz=0
can't find file to patch at input line 3
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|--- unit-tests/phpunit.xml.orig 2013-07-15 22:43:31.311938857 +0200
|+++ unit-tests/phpunit.xml.orig 2013-07-15 22:44:08.794605062 +0200
--------------------------
File to patch: 
Skip this patch? [y] 
Skipping patch.
3 out of 3 hunks ignored
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.V3GSMX (%prep)
    Bad exit status from /var/tmp/rpm-tmp.V3GSMX (%prep)
Child return code was: 1
EXCEPTION: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target x86_64 --nodeps builddir/build/SPECS/php-phalcon.spec']
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/mockbuild/trace_decorator.py", line 70, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.7/site-packages/mockbuild/util.py", line 359, in do
    raise mockbuild.exception.Error, ("Command failed. See logs for output.\n # %s" % (command,), child.returncode)
Error: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target x86_64 --nodeps builddir/build/SPECS/php-phalcon.spec']
LEAVE do --> EXCEPTION RAISED

Ok, removed .orig from the header of the file and I got this now:

Patch #0 (php-phalcon-phpunit.patch):
+ /usr/bin/cat /builddir/build/SOURCES/php-phalcon-phpunit.patch
+ /usr/bin/patch -p0 --fuzz=0
patching file unit-tests/phpunit.xml
Hunk #1 FAILED at 14.
Hunk #2 FAILED at 31.
Hunk #3 FAILED at 63.
3 out of 3 hunks FAILED -- saving rejects to file unit-tests/phpunit.xml.rej
error: Bad exit status from /var/tmp/rpm-tmp.V04NF8 (%prep)
RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.V04NF8 (%prep)
Child return code was: 1
EXCEPTION: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target x86_64 --nodeps builddir/build/SPECS/php-phalcon.spec']
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/mockbuild/trace_decorator.py", line 70, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.7/site-packages/mockbuild/util.py", line 359, in do
    raise mockbuild.exception.Error, ("Command failed. See logs for output.\n # %s" % (command,), child.returncode)
Error: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target x86_64 --nodeps builddir/build/SPECS/php-phalcon.spec']
LEAVE do --> EXCEPTION RAISED

Created attachment 776248 [details]
better description and updated spec file for running unittests

(In reply to Renich Bon Ciric from comment #21)
> (In reply to Roman Mohr from comment #20)
> > Created attachment 773914 [details]
> > Patch to run most of the non-db-dependent unit tests
> > 
> > Apply in combination with attachment 773907 [details]
> 
> Thank you, Roman, for your hard work and collaboration.
> 

You are welcome :)

> Here's what I got:
> 
> Patch #0 (php-phalcon-phpunit.patch):
> + /usr/bin/cat /builddir/build/SOURCES/php-phalcon-phpunit.patch
> + /usr/bin/patch -p0 --fuzz=0
> can't find file to patch at input line 3
> Perhaps you used the wrong -p or --strip option?
> The text leading up to this was:
> --------------------------
> |--- unit-tests/phpunit.xml.orig 2013-07-15 22:43:31.311938857 +0200
> |+++ unit-tests/phpunit.xml.orig 2013-07-15 22:44:08.794605062 +0200
> --------------------------
Strange, where does that header com from? The header of the uploaded patch looks like this:

--- unit-tests/phpunit.xml.orig	2013-07-15 22:43:31.311938857 +0200
+++ unit-tests/phpunit.xml	2013-07-15 22:44:08.794605062 +0200

It should be enough that the correct file name is in the '+++' line.

(In reply to Renich Bon Ciric from comment #22)
> Ok, removed .orig from the header of the file and I got this now:
> 
> Patch #0 (php-phalcon-phpunit.patch):
> + /usr/bin/cat /builddir/build/SOURCES/php-phalcon-phpunit.patch
> + /usr/bin/patch -p0 --fuzz=0
> patching file unit-tests/phpunit.xml
> Hunk #1 FAILED at 14.
> Hunk #2 FAILED at 31.
> Hunk #3 FAILED at 63.

The official github tag is not commit b72745 used by us. I am quite sure that you are using an old archive. Please place the correct source tar.gz in your SOURCES folder.

I have uploaded a new spec patch with
 - improved description
 - removed CFLAGS
 - improved (architecture independent) builddir detection
 - removed architecture limitation

There is just one point left in my eyes: I see no reason why phalcon should not run on ppc and arm. If you know of any reason then readd the exclusive arch directive and add a comment why it won't run.

(In reply to Roman Mohr from comment #23)
> Created attachment 776248 [details]
> better description and updated spec file for running unittests
> 
> (In reply to Renich Bon Ciric from comment #21)
> > (In reply to Roman Mohr from comment #20)
> > > Created attachment 773914 [details]
> > > Patch to run most of the non-db-dependent unit tests
> > > 
> > > Apply in combination with attachment 773907 [details]
> > 
> > Thank you, Roman, for your hard work and collaboration.
> > 
> 
> You are welcome :)
> 
> > Here's what I got:
> > 
> > Patch #0 (php-phalcon-phpunit.patch):
> > + /usr/bin/cat /builddir/build/SOURCES/php-phalcon-phpunit.patch
> > + /usr/bin/patch -p0 --fuzz=0
> > can't find file to patch at input line 3
> > Perhaps you used the wrong -p or --strip option?
> > The text leading up to this was:
> > --------------------------
> > |--- unit-tests/phpunit.xml.orig 2013-07-15 22:43:31.311938857 +0200
> > |+++ unit-tests/phpunit.xml.orig 2013-07-15 22:44:08.794605062 +0200
> > --------------------------
> Strange, where does that header com from? The header of the uploaded patch
> looks like this:
> 
> --- unit-tests/phpunit.xml.orig	2013-07-15 22:43:31.311938857 +0200
> +++ unit-tests/phpunit.xml	2013-07-15 22:44:08.794605062 +0200
> 
> It should be enough that the correct file name is in the '+++' line.
> 
> (In reply to Renich Bon Ciric from comment #22)
> > Ok, removed .orig from the header of the file and I got this now:
> > 
> > Patch #0 (php-phalcon-phpunit.patch):
> > + /usr/bin/cat /builddir/build/SOURCES/php-phalcon-phpunit.patch
> > + /usr/bin/patch -p0 --fuzz=0
> > patching file unit-tests/phpunit.xml
> > Hunk #1 FAILED at 14.
> > Hunk #2 FAILED at 31.
> > Hunk #3 FAILED at 63.
> 
> The official github tag is not commit b72745 used by us. I am quite sure
> that you are using an old archive. Please place the correct source tar.gz in
> your SOURCES folder.

Well, actually, we're using a tag now: v1.2.0. Also, I am using the spectool you taught me.

I re-did the patch and I had to comment out several more Db tests; as well as some Collections and stuff.

Anyway, the patch is remade in the SRPM and it now builds correctly.

> I have uploaded a new spec patch with
>  - improved description
>  - removed CFLAGS
>  - improved (architecture independent) builddir detection
>  - removed architecture limitation
> 
> There is just one point left in my eyes: I see no reason why phalcon should
> not run on ppc and arm. If you know of any reason then readd the exclusive
> arch directive and add a comment why it won't run.

I've emailed the author regarding these issues. I, also, asked his help in reviewing the review; so he can clarify any pending points (like the CFLAGS) and other matters.

Thank you once again. I appreciate your great effort and patience.

Latest spec file and srpm:

SPEC: http://dev1.woralelandia.com/~renich/SPECS/php-phalcon.spec
SRPM: http://dev1.woralelandia.com/~renich/SRPMS/php-phalcon-1.2.0-8.fc18.src.rpm

> > The official github tag is not commit b72745 used by us. I am quite sure
> > that you are using an old archive. Please place the correct source tar.gz in
> > your SOURCES folder.
> 
> Well, actually, we're using a tag now: v1.2.0. Also, I am using the spectool
> you taught me.

Good, I just wanted to make sure this is the case.

> I re-did the patch and I had to comment out several more Db tests; as well
> as some Collections and stuff.
> 
> Anyway, the patch is remade in the SRPM and it now builds correctly.

I still think that you mixed up some patches. Anyway I am glad it works for you.

> > I have uploaded a new spec patch with
> >  - improved description
> >  - removed CFLAGS
> >  - improved (architecture independent) builddir detection
> >  - removed architecture limitation
> > 
> > There is just one point left in my eyes: I see no reason why phalcon should
> > not run on ppc and arm. If you know of any reason then readd the exclusive
> > arch directive and add a comment why it won't run.
> 
> I've emailed the author regarding these issues. I, also, asked his help in
> reviewing the review; so he can clarify any pending points (like the CFLAGS)
> and other matters.

Good to hear that you are in close contact with upstream. Once you have your answeres we can finalize the specs. (btw. 32bit and 64bit build directory does not mean that a software is just meant for x86 cpu's and on github you can read "Phalcon PHP is written in C with platform independence in mind. As a result, Phalcon PHP is available on Microsoft Windows, GNU/Linux, Mac OS X", which sounds for me like it schould used wherever possible. And you can trust me, the cflags are just optimization flags ;) ).

> Thank you once again. I appreciate your great effort and patience.

Just drop me a note when you have some more infos.

Hey Roman!

Sorry; got lost in work! Also, lost my dev1 server. Do you happen to have a copy of the most current SRPM?

Anyway, quick answers:

- The necessary flags are CFLAGS="-O2 -fno-delete-null-pointer-checks"
- I'll see what's up with other arch support. Hopefuly, they are buildable.

Ok, upstream said (paraphrased): "I've never tried compiling in PPC or ARM; so I don't know"

Let's just remove that restriction and see what happens. If there is a way of marking non X86/X86_64 arches as experimental, it would be cool.

Btw, if anyone has an SRPM, I'd appreciate it. Otherwize, gotta restart the SPEC.

(In reply to Renich Bon Ciric from comment #28)
> Ok, upstream said (paraphrased): "I've never tried compiling in PPC or ARM;
> so I don't know"
> 
> Let's just remove that restriction and see what happens. If there is a way
> of marking non X86/X86_64 arches as experimental, it would be cool.
> 
Hi Reich,
I still have them. I will send you an email tonight.
> Btw, if anyone has an SRPM, I'd appreciate it. Otherwize, gotta restart the
> SPEC.

Ok, thanks Roman!

So, now what? Ready to deploy?

When you think the spec file is done, (In reply to Renich Bon Ciric from comment #30)
> Ok, thanks Roman!
> 
> So, now what? Ready to deploy?

When you think the spec file and the srpm for rawhide are ready, post them again (best with koji build) and I will review the spec again. If it passes the review you can request a git repository.

Cheers,
Roman

Ok, here's the latest upstream version.

SPEC: http://renich.fedorapeople.org/SPECS/php-phalcon.spec
SRPM: http://renich.fedorapeople.org/SRPMS/php-phalcon-1.2.3-1.fc19.src.rpm

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Package requires php-common instead of php.
  See: http://fedoraproject.org/wiki/Packaging:PHP


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[-]: 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.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (3 clause)", "MIT/X11 (BSD like)", "Unknown or generated". 939 files
     have unknown license. Detailed output of licensecheck in
     /home/roman/rpmbuild/REVIEWS/977208-php-phalcon/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: If the package is under multiple licenses, the licensing breakdown must
     be documented in the spec.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 71680 bytes in 6 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[-]: Fully versioned dependency in subpackages, if present.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[!]: Package should compile and build into binary rpms on all supported
     architectures.

     ---> hangs in %check at 'AssetsTest::testJsminFilter'. See 
          https://koji.fedoraproject.org/koji/taskinfo?taskID=5978275.

[!]: %check is present and all tests pass.

     ---> only tests which require a database are disabled. On arm the build
          hangs in %check section, on all other platform the tests pass.

[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: php-phalcon-1.2.3-1.fc21.x86_64.rpm
php-phalcon.x86_64: W: spelling-error %description -l en_US backends -> back ends, back-ends, backhands
php-phalcon.x86_64: W: spelling-error %description -l en_US pecl -> peck, peel, pecs
php-phalcon.x86_64: W: spelling-error %description -l en_US mongo -> mono, mongol, mango
php-phalcon.x86_64: W: spelling-error %description -l en_US mysql -> myself
php-phalcon.x86_64: W: spelling-error %description -l en_US pgsql 
php-phalcon.x86_64: W: spelling-error %description -l en_US memcached -> schemed
1 packages and 0 specfiles checked; 0 errors, 6 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint php-phalcon
php-phalcon.x86_64: W: spelling-error %description -l en_US backends -> back ends, back-ends, backhands
php-phalcon.x86_64: W: spelling-error %description -l en_US pecl -> peck, peel, pecs
php-phalcon.x86_64: W: spelling-error %description -l en_US mongo -> mono, mongol, mango
php-phalcon.x86_64: W: spelling-error %description -l en_US mysql -> myself
php-phalcon.x86_64: W: spelling-error %description -l en_US pgsql 
php-phalcon.x86_64: W: spelling-error %description -l en_US memcached -> schemed
1 packages and 0 specfiles checked; 0 errors, 6 warnings.
# echo 'rpmlint-done:'



Requires
--------
php-phalcon (rpmlib, GLIBC filtered):
    config(php-phalcon)
    libc.so.6()(64bit)
    php(api)
    php(zend-abi)
    rtld(GNU_HASH)



Provides
--------
php-phalcon:
    config(php-phalcon)
    php-phalcon
    php-phalcon(x86-64)



Unversioned so-files
--------------------
php-phalcon: /usr/lib64/php/modules/phalcon.so

Source checksums
----------------
https://github.com/phalcon/cphalcon/archive/v1.2.3.tar.gz#/php-phalcon-1.2.3.tar.gz :
  CHECKSUM(SHA256) this package     : e8630c78416264b99af9d5205d1840d1a4383c4416c6e4cc887987f8466b7afc
  CHECKSUM(SHA256) upstream package : e8630c78416264b99af9d5205d1840d1a4383c4416c6e4cc887987f8466b7afc


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -b 977208 -m fedora-rawhide-x86_64

###

We should investigate why this single unittest hangs when building on arm.

@Björn: As this is just a 'SHOULD' item, I would tend to approve the package, what do you say?

Hey dudes!

Upstream contacted me suggesting to update while mentioning it might help with the ARM issues.

SPEC: http://renich.fedorapeople.org/SPECS/php-phalcon.spec
SRPM: http://renich.fedorapeople.org/SRPMS/php-phalcon-1.2.4-1.fc19.src.rpm

ping

Legal issue.

From ext/assets/filters/jsminifier.h

   Copyright (c) 2002 Douglas Crockford  (www.crockford.com)
   The Software shall be used for Good, not Evil.

This license is not free and so cannot be used in Fedora.

(In reply to Remi Collet from comment #36)
> Legal issue.
> 
> From ext/assets/filters/jsminifier.h
> 
>    Copyright (c) 2002 Douglas Crockford  (www.crockford.com)
>    The Software shall be used for Good, not Evil.
> 
> This license is not free and so cannot be used in Fedora.

Ah, I'll get in contact with upstream to see if this can be solved.

(In reply to Renich Bon Ciric from comment #34)
> Hey dudes!
> 
> Upstream contacted me suggesting to update while mentioning it might help
> with the ARM issues.
> 
> SPEC: http://renich.fedorapeople.org/SPECS/php-phalcon.spec
> SRPM: http://renich.fedorapeople.org/SRPMS/php-phalcon-1.2.4-1.fc19.src.rpm

good news, the unittests pass on every architecture! :D

http://koji.fedoraproject.org/koji/taskinfo?taskID=6195054

In my eyes the package is ready when the copyright issue is resolved.

FYI
https://github.com/phalcon/cphalcon/pull/2772
https://github.com/phalcon/cphalcon/pull/2774
https://github.com/phalcon/cphalcon/pull/2775
https://raw.githubusercontent.com/remicollet/remirepo/master/php/php-phalcon/php-phalcon.spec

Wow, this eems great. Why didn't I notice this before? Hmmm...

@Remi
Anyway, what's next? Will you package it or should we co-maintain?

(In reply to Renich Bon Ciric from comment #40)

> @Remi
> Anyway, what's next? Will you package it or should we co-maintain?

This is your review ;)

I just want to raise your attention that the non-free issue is now fixed upstream (will be in 1.3.3, all my PR have be merged) so you can start updating your spec and go with the review.

Of course, you can take some changes from my spec file (strip.sh, genbuild in %prep, ZTS build...) I only run test from ext/texts, but the PHPUnit suite should also be run, IIUC this should be the only one in 2.x)

Notice: instead of patching the phpunit configuration file, it will be simpler to use the --exclude-group option (and add the @group annotation in the unit tests, change which can be submitted upstream)

If you want I can also be co-maintainer. If you use this framework, you are probably a better owner than me (I mostly work on it to fix the PHP 5.6 compatibility). And I already own enough packages ;)

Hi,

What is the status on this?

Benjamin

If some people want this extension, the package is available in my repository (latest version is php-phalcon3-3.3.1), btw I don't plan to submit it for official repository (non-free issue still not fixed upstream).

This review can probably be closed as "dead"

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

Do you still intend to complete this review? If so, please fix any outstanding issues (non-free being the only remaining issue if I read the above comments correctly?) and submit new specfile and srpm. I can review if earlier reviewers are not available anymore, If not, please close this bug or do nothing, in which case automation will close it in one month.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.