Bug 728956

As this package don't work,, I can't do the review for now.

First notes:

Don't need to require a "webserver" as this application could be, as all web-app, installed on 1 server and used from another workstation.

According to PHP Compatinfo analysis should requires php-gd and php-mysql
$ phpci print --report=extension --recursive php-virt-control-0.0.2/
33 / 33 [+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++>] 100.00%
BASE: /home/extras/SPECS/review/php-virt-control-0.0.2
-------------------------------------------------------------------------------
PHP COMPAT INFO EXTENSION SUMMARY
-------------------------------------------------------------------------------
  EXTENSION                                        PECL   VERSION         COUNT
-------------------------------------------------------------------------------
  Core                                                    4.0.0             238
  date                                                    4.0.0               1
  gd                                                      4.0.0              14
  mysql                                              1.0  4.0.0               9
  session                                                 4.0.0               1
  standard                                                4.0.0             411
-------------------------------------------------------------------------------
A TOTAL OF 6 EXTENSION(S) WERE FOUND
REQUIRED PHP 4.0.0 (MIN) 

Should also requires "php" (and so, httpd)

Please add "Allow From ::1" in php-virt-control.conf

This app use <?=
This requires the short_open_tag deprecated and discouraged option.

So 
- add in php-virt-control.conf (only for this app)
    php-flag short_open_tag On
- report this as a bug to upstream

On first call to http://localhost/php-virt-control/
Warning: libvirt_check_version(): Invalid version type in /usr/share/php-virt-control/init.php on line 51 Call Stack: 
0.0003 659224 1. {main}() /usr/share/php-virt-control/index.php:0 
0.0003 659664 2. require('/usr/share/php-virt-control/init.php') /usr/share/php-virt-control/index.php:2 
0.0009 791704 3. libvirt_check_version() /usr/share/php-virt-control/init.php:51 

And => 
Update needed:
Your version of libvirt-php module is too old for this application to work properly. Please upgrade your libvirt-php version from http://libvirt.php first.

Despite I have installed the latest version available
$ rpm -q php-libvirt 
php-libvirt-0.4.3-1.fc15 (x86_64)
$ php -r 'print_r( libvirt_version() );'
Array
(
    [libvirt.release] => 8
    [libvirt.minor] => 8
    [libvirt.major] => 0
    [connector.version] => 0.4.3
    [connector.major] => 0
    [connector.minor] => 4
    [connector.release] => 3
)

The name "php-virt-control" doesn't seems really well choosen.

For packaging, ok, it use the upstream project name, but it seems to be a php extension instead of a web app.

For project name, please read "Use of the "PHP" name" in
http://fr2.php.net/license/index.php#faq-lic

This should probably be discussed with upstream.

(In reply to comment #2)
> The name "php-virt-control" doesn't seems really well choosen.
> 
> For packaging, ok, it use the upstream project name, but it seems to be a php
> extension instead of a web app.
> 
> For project name, please read "Use of the "PHP" name" in
> http://fr2.php.net/license/index.php#faq-lic
> 
> This should probably be discussed with upstream.

Well, I'm maintaining the upstream for php-virt-control myself. Also, I've modified the SPEC file and SRPM and also fixed the short open tags as based on your comment #1 and everything seems to be working fine on my F-14 i686 box and I was unable to run into issues you mentioned in comment #1.

Nevertheless the new update for php-libvirt have been submitted so you may try once the update is available.

Michl

Created attachment 519478 [details]
Fix version check

Same error with php-libvirt 0.4.4

The attached patched seems to solve this issue.

I still not able to run this app.

The INSTALL file provided is absolutely irrelevant (configure/make/install)

Please provide an minimal howto.

After filing the data/mysql_conn.php, I get a blank page.

About packaging, data/mysql_conn.php must go to a subfolder of /etc/ and be tagged as %config.

(In reply to comment #4)
> Created attachment 519478 [details]
> Fix version check
> 
> Same error with php-libvirt 0.4.4
> 
> The attached patched seems to solve this issue.

Remi, what system are you trying this on? I'm on Fedora-14 i386 box and my PHP version is php-5.3.6-1.fc14.i686.

The php-libvirt package comes with the tests when you build it from upstream git repository and for some version of PHP those tests fails according to my testing. The issue is that my version of PHP accepts optional arguments that are really optional however it fails for some other version of PHP which creates the issue.

Thanks for your information,
Michal

(In reply to comment #6)
> Remi, what system are you trying this on? I'm on Fedora-14 i386 box and my PHP
> version is php-5.3.6-1.fc14.i686.

I use php 5.3.8 (in updates-testing), under f15.x86_64

(In reply to comment #7)
> (In reply to comment #6)
> > Remi, what system are you trying this on? I'm on Fedora-14 i386 box and my PHP
> > version is php-5.3.6-1.fc14.i686.
> 
> I use php 5.3.8 (in updates-testing), under f15.x86_64

I didn't test using this version in updates. It's strange how it doesn't work on this version. Could you please test on 5.3.6 version of PHP as well or at least clone php-libvirt directly from git and try to issue 'make check' to run tests and see whether they're going fine or fails?

Thanks,
Michal

(In reply to comment #8)
> clone php-libvirt directly from git and try to issue 'make check' to run
> tests and see whether they're going fine or fails?
$ make check
...
./runtests.sh
Test test-connect.phpt has been completed successfully
Test test-version-check.phpt has been completed successfully
Test test-version-get.phpt has been completed successfully
Test test-domain-define-undefine.phpt has been completed successfully
Test test-domain-define-create-destroy.phpt has been completed successfully
Test test-domain-create.phpt has been completed successfully
Test test-domain-create-and-get-xpath.phpt has been completed successfully
Test test-domain-create-and-coredump.phpt has been completed successfully

Warning: unlink(test.log): No such file or directory in /home/remi/GIT/libvirt-php/tests/test-logging.phpt on line 6

Call Stack:
    0.0002     677768   1. {main}() /home/remi/GIT/libvirt-php/tests/test-logging.phpt:0
    0.0003     684600   2. unlink() /home/remi/GIT/libvirt-php/tests/test-logging.phpt:6

Test test-logging.phpt has been completed successfully

Warning: libvirt_connect(): Maximum number of connections allowed exceeded in /home/remi/GIT/libvirt-php/tests/test-conn-limit.phpt on line 17

Call Stack:
    0.0001     655616   1. {main}() /home/remi/GIT/libvirt-php/tests/test-conn-limit.phpt:0
    0.1744     696400   2. libvirt_connect() /home/remi/GIT/libvirt-php/tests/test-conn-limit.phpt:17

Test test-conn-limit.phpt has been completed successfully

Warning: libvirt_domain_snapshot_create(): erreur interne unable to execute QEMU command 'savevm': The command savevm has not been found in /home/remi/GIT/libvirt-php/tests/test-domain-snapshot.phpt on line 18

Call Stack:
    0.0002     676728   1. {main}() /home/remi/GIT/libvirt-php/tests/test-domain-snapshot.phpt:0
    0.4048     686504   2. libvirt_domain_snapshot_create() /home/remi/GIT/libvirt-php/tests/test-domain-snapshot.phpt:18




NB : in tests/test-version-check.phpt, all calls to libvirt_check_version set the forth option (what is my patch doing)
[Error 1] Error on creating snapshot: erreur interne unable to execute QEMU command 'savevm': The command savevm has not been found
make[1]: *** [check] Erreur 1
make[1] : on quitte le répertoire « /home/remi/GIT/libvirt-php/tests »
make: *** [check-recursive] Erreur 1

(In reply to comment #9)
> (In reply to comment #8)
> > clone php-libvirt directly from git and try to issue 'make check' to run
> > tests and see whether they're going fine or fails?
> $ make check
> ...
> ./runtests.sh
> Test test-connect.phpt has been completed successfully
> Test test-version-check.phpt has been completed successfully
> Test test-version-get.phpt has been completed successfully
> Test test-domain-define-undefine.phpt has been completed successfully
> Test test-domain-define-create-destroy.phpt has been completed successfully
> Test test-domain-create.phpt has been completed successfully
> Test test-domain-create-and-get-xpath.phpt has been completed successfully
> Test test-domain-create-and-coredump.phpt has been completed successfully
> 
> Warning: unlink(test.log): No such file or directory in
> /home/remi/GIT/libvirt-php/tests/test-logging.phpt on line 6
> 
> Call Stack:
>     0.0002     677768   1. {main}()
> /home/remi/GIT/libvirt-php/tests/test-logging.phpt:0
>     0.0003     684600   2. unlink()
> /home/remi/GIT/libvirt-php/tests/test-logging.phpt:6
> 
> Test test-logging.phpt has been completed successfully
> 
> Warning: libvirt_connect(): Maximum number of connections allowed exceeded in
> /home/remi/GIT/libvirt-php/tests/test-conn-limit.phpt on line 17
> 
> Call Stack:
>     0.0001     655616   1. {main}()
> /home/remi/GIT/libvirt-php/tests/test-conn-limit.phpt:0
>     0.1744     696400   2. libvirt_connect()
> /home/remi/GIT/libvirt-php/tests/test-conn-limit.phpt:17
> 
> Test test-conn-limit.phpt has been completed successfully
> 
> Warning: libvirt_domain_snapshot_create(): erreur interne unable to execute
> QEMU command 'savevm': The command savevm has not been found in
> /home/remi/GIT/libvirt-php/tests/test-domain-snapshot.phpt on line 18
> 
> Call Stack:
>     0.0002     676728   1. {main}()
> /home/remi/GIT/libvirt-php/tests/test-domain-snapshot.phpt:0
>     0.4048     686504   2. libvirt_domain_snapshot_create()
> /home/remi/GIT/libvirt-php/tests/test-domain-snapshot.phpt:18
> 
> 
> 
> 
> NB : in tests/test-version-check.phpt, all calls to libvirt_check_version set
> the forth option (what is my patch doing)
> [Error 1] Error on creating snapshot: erreur interne unable to execute QEMU
> command 'savevm': The command savevm has not been found
> make[1]: *** [check] Erreur 1
> make[1] : on quitte le répertoire « /home/remi/GIT/libvirt-php/tests »
> make: *** [check-recursive] Erreur 1

That's strange. Seems like to be the issue of php-libvirt for your version of PHP since the version check is working fine for me. I need to double-check in php-libvirt sources then. The not working libvirt_check_version() call is basically a php-libvirt bug.

Michal

Created attachment 519630 [details]
php-libvirt-check.patch

This patch solves the x86_64 check_version issue.

I think most test which are available and which could run during rpm build should be run in %check.

Another small issue :
mak check run test against the installed version, not against the build one.

(In reply to comment #11)
> Created attachment 519630 [details]
> php-libvirt-check.patch
> 
> This patch solves the x86_64 check_version issue.
> 
> I think most test which are available and which could run during rpm build
> should be run in %check.
> 
> Another small issue :
> mak check run test against the installed version, not against the build one.

Thanks for the patch Remi! I've applied it to the git tree now!

Thanks again!
Michal

Remi, did you test the php-virt-control with the latest codebase from http://libvirt.org/git/?p=libvirt-php.git;a=summary (i.e. with your x86_64 patch applied)? Any feedback on this?

Thanks,
Michal

(In reply to comment #13)
> Remi, did you test the php-virt-control with the latest codebase from
> http://libvirt.org/git/?p=libvirt-php.git;a=summary (i.e. with your x86_64
> patch applied)? Any feedback on this?

See my comment 5

(In reply to comment #14)
> (In reply to comment #13)
> > Remi, did you test the php-virt-control with the latest codebase from
> > http://libvirt.org/git/?p=libvirt-php.git;a=summary (i.e. with your x86_64
> > patch applied)? Any feedback on this?
> 
> See my comment 5

Thanks! I did update the version to 0.0.2-2 with the spec file and rest modified to have an updated version of INSTALL file as well as the configuration files moved to the /etc/php-virt-control directory.

Locations are:
SPEC file: http://php-virt-control.org/download/php-virt-control.spec
SRPM file: http://php-virt-control.org/download/php-virt-control-0.0.2-2.fc14.src.rpm 
tar archive: http://php-virt-control.org/download/php-virt-control-0.0.2.tar.gz

Hope this is fine now!

Thanks,
Michal

Remi, did you test using those packages? If you still have issues, could you please provide me more information on setup you try to run php-virt-control on? This means operating system with version, apache version and php version used for the testing so I can try it myself (although in VM so not all functionality will work)?

Thanks,
Michal

Remi, since now I installed F-15 on my personal laptop I did try to see what issues are there and there were still some issues so I fixed them. The version is not 0.0.2-3 and several bugfixes for Fedora-15 are present in this version.

Locations are:
SPEC file: http://php-virt-control.org/download/php-virt-control.spec
SRPM file:
http://php-virt-control.org/download/php-virt-control-0.0.2-3.src.rpm 
tar archive: http://php-virt-control.org/download/php-virt-control-0.0.2.tar.gz

Hope works for you fine as well. It at least works for me on my F-15 i386 box.

Thanks,
Michal

Michal, I'd really suggest to rename the package and upstream to drop the PHP from the name. Either drop it altogether or rename it to web-virt-control to make it clear that this is a web application.

(In reply to comment #18)
> Michal, I'd really suggest to rename the package and upstream to drop the PHP
> from the name. Either drop it altogether or rename it to web-virt-control to
> make it clear that this is a web application.

Well, according to the Fedora packaging policy for PHP application it should start with "php-" from what I know, shouldn't it ? Also, I'd like to have the PHP stated in here to identify the system is PHP-based.

Michal

Remi already wrote it above - php-<something> must be used for the PHP addon modules - for example for the php-libvirt it is an appropriate name. However for the end applications that just happen to be written using PHP there is no such requirement. And IMO it is much more important to know that it is a web application - thus my suggestion to rename it to web-virt-control - than the language in which it is written. And it also conflict with the request (of course not mandatory requirement) of PHP project to not use the PHP in the name of applications written using PHP.

(In reply to comment #20)
> Remi already wrote it above - php-<something> must be used for the PHP addon
> modules - for example for the php-libvirt it is an appropriate name. However
> for the end applications that just happen to be written using PHP there is no
> such requirement. And IMO it is much more important to know that it is a web
> application - thus my suggestion to rename it to web-virt-control - than the
> language in which it is written. And it also conflict with the request (of
> course not mandatory requirement) of PHP project to not use the PHP in the name
> of applications written using PHP.

Ok, right. Now I see the point however I got inspired by phpMyAdmin which is basically a web application. So in fact if the inspiration was bad (however the name is working for them for a pretty long time already) then I should consider renaming it at least for Fedora name.

Michal

As you're upstream as I understand it please either rename it both in upstream and as a package if possible. I know that phpMyAdmin is the same case however there the name was set up long ago and changing it now makes no sense anymore. Your project is not yet established firmly so I'd suggest changing the name sooner than later.

(In reply to comment #22)
> As you're upstream as I understand it please either rename it both in upstream
> and as a package if possible. I know that phpMyAdmin is the same case however
> there the name was set up long ago and changing it now makes no sense anymore.
> Your project is not yet established firmly so I'd suggest changing the name
> sooner than later.

I can see the point however I need to point out that there are some issues that the name change would present. Consider the fact that I did buy a domain php-virt-control.org for 5 years on my own expenses as well as the project's git is hosted on server that's not under my management by root user to rename the repository. Although I think Daniel would be fine with renaming it I guess having the domain as php-virt-control.org with the web-virt-control project would be pretty confusing for the visitors/people looking for such web application. I guess the name of php-virt-control would be easy to remember now than to remember php-virt-control.org site with the web-virt-control project on it. The issue is that the domain has been bought for the project already and it's been bought for 5 years (let's check the whois database for php-virt-control.org).

Michal

Ok, after thinking about this and having the facts already mentioned I don't like the idea of renaming upstream however it should be fine both parties to change the Fedora name to web-virt-control so I have to ask you whether is that fine with you.

Thanks,
Michal

I do not think it makes sense to rename the Fedora package if upstream stays at the php-virt-control name. Strictly said the name as it is does not break the Fedora naming guidelines so there is no requirement for it to be changed to pass the Fedora review. It was just my suggestion to you to rename it upstream to fulfil the wish of the PHP upstream and to make the name more descriptive.

(In reply to comment #25)
> I do not think it makes sense to rename the Fedora package if upstream stays at
> the php-virt-control name. Strictly said the name as it is does not break the
> Fedora naming guidelines so there is no requirement for it to be changed to
> pass the Fedora review. It was just my suggestion to you to rename it upstream
> to fulfil the wish of the PHP upstream and to make the name more descriptive.

Tom, I know you mean it good way but considering the fact I did buy a domain on my own expenses (although hosted on Daniel's server) rendering the project my 'spare time' project I don't like the renaming. The situation would be different if I didn't have the domain bought for 5 years yet however the situation is different with me having it already bought.

So is that ok to review it now or what should I do? The reason why I don't like the idea of renaming the project is already mentioned and I don't want to buy a new domain just because of the name change as the DNS record matches exactly the upstream project name.

Michal

Yeah, the name is not blocking the review. You can keep the name as is.

(In reply to comment #27)
> Yeah, the name is not blocking the review. You can keep the name as is.

That's fine so I'll wait until somebody does the review. I see you're busy but it's OK for me to wait since I'm pretty busy these days as well.

Michal

New version, 0.0.3, is available at:

SPEC file: http://php-virt-control.org/download/php-virt-control.spec
SRPM file:
http://php-virt-control.org/download/php-virt-control-0.0.3.src.rpm
tar archive: http://php-virt-control.org/download/php-virt-control-0.0.3.tar.gz

Michal

Any news on this Remi?

Thanks!
Michal

I'll do the final review.

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5366174

$ rpmlint -i -v *
php-virt-control.i686: I: checking
php-virt-control.i686: W: spelling-error %description -l en_US libvirt -> liberty
The value of this tag appears to be misspelled. Please double-check.

php-virt-control.i686: W: incoherent-version-in-changelog 0.0.3 ['0.0.3-1.fc20', '0.0.3-1']
The latest entry in %changelog contains a version identifier that is not
coherent with the epoch:version-release tuple of the package.

php-virt-control.i686: I: checking-url http://www.php-virt-control.org (timeout 10 seconds)
php-virt-control.i686: W: unstripped-binary-or-object /usr/bin/apache-key-copy
php-virt-control.i686: W: no-manual-page-for-binary apache-key-copy
Each executable in standard binary directories should have a man page.

php-virt-control.i686: W: install-file-in-docs /usr/share/doc/php-virt-control-0.0.3/INSTALL
A file whose name suggests that it contains installation instructions is
included in the package.  Such instructions are often not relevant for already
installed packages; if this is the case for this file and it does not contain
any information that is of interest after the package has been built and
installed, do not include the file in the binary package.

php-virt-control.src: I: checking
php-virt-control.src: W: spelling-error %description -l en_US libvirt -> liberty
The value of this tag appears to be misspelled. Please double-check.

php-virt-control.src: I: checking-url http://www.php-virt-control.org (timeout 10 seconds)
php-virt-control.src:27: W: macro-in-comment %{summary}
There is a unescaped macro after a shell style comment in the specfile. Macros
are expanded everywhere, so check if it can cause a problem in this case and
escape the macro with another leading % if appropriate.

php-virt-control.src: W: no-%build-section
The spec file does not contain a %build section.  Even if some packages don't
directly need it, section markers may be overridden in rpm's configuration to
provide additional "under the hood" functionality, such as injection of
automatic -debuginfo subpackages.  Add the section, even if empty.

php-virt-control.src: I: checking-url http://www.php-virt-control.org/download/php-virt-control-0.0.3.tar.gz (timeout 10 seconds)
php-virt-control.src: W: file-size-mismatch php-virt-control-0.0.3.tar.gz = 139067, http://www.php-virt-control.org/download/php-virt-control-0.0.3.tar.gz = 133890
The size of the file in the package does not match the size indicated by
peeking at its URL.  Verify that the file in the package has the intended
contents.

php-virt-control.x86_64: I: checking
php-virt-control.x86_64: W: spelling-error %description -l en_US libvirt -> liberty
The value of this tag appears to be misspelled. Please double-check.

php-virt-control.x86_64: W: incoherent-version-in-changelog 0.0.3 ['0.0.3-1.fc20', '0.0.3-1']
The latest entry in %changelog contains a version identifier that is not
coherent with the epoch:version-release tuple of the package.

php-virt-control.x86_64: I: checking-url http://www.php-virt-control.org (timeout 10 seconds)
php-virt-control.x86_64: W: unstripped-binary-or-object /usr/bin/apache-key-copy
php-virt-control.x86_64: W: no-manual-page-for-binary apache-key-copy
Each executable in standard binary directories should have a man page.

php-virt-control.x86_64: W: install-file-in-docs /usr/share/doc/php-virt-control-0.0.3/INSTALL
A file whose name suggests that it contains installation instructions is
included in the package.  Such instructions are often not relevant for already
installed packages; if this is the case for this file and it does not contain
any information that is of interest after the package has been built and
installed, do not include the file in the binary package.

php-virt-control.spec:27: W: macro-in-comment %{summary}
There is a unescaped macro after a shell style comment in the specfile. Macros
are expanded everywhere, so check if it can cause a problem in this case and
escape the macro with another leading % if appropriate.

php-virt-control.spec: W: no-%build-section
The spec file does not contain a %build section.  Even if some packages don't
directly need it, section markers may be overridden in rpm's configuration to
provide additional "under the hood" functionality, such as injection of
automatic -debuginfo subpackages.  Add the section, even if empty.

php-virt-control.spec: I: checking-url http://www.php-virt-control.org/download/php-virt-control-0.0.3.tar.gz (timeout 10 seconds)
3 packages and 1 specfiles checked; 0 errors, 16 warnings.



The spelling error can be safely ignored.

Please drop INSTALL from the docs, it's senseless to ship general installation instructions within a rpm package. Moreover, you don't use the described autoconf/automake stuff to build the package anyway.

Although it wouldn't change anything for the time being, please add an empty %build section.

The release number is missing from your latest changelog entry.

What about the differing file sizes of the original and the packaged tarball? What happened here? The files must not differ, and unless you are packaging a VCS checkout, the checksums have to be identical.

One runtime requirement is redundant and can be dropped:
httpd (needed by php)

Your package contains mostly pure PHP code, there's just one file which is not arch-independent. I propose to provide a -common package which contains all the PHP stuff, the configuration and doc files and so on, and this has to be "BuildArch: noarch". Then, the main package remains with the file /usr/bin/apache-key-copy only.

php-virt-control.x86_64: W: unstripped-binary-or-object /usr/bin/apache-key-copy
This needs to be investigated.

No idea why this dropped back into the queue; it should have just been closed.