Bug 1411188 - Review Request: php-echonest-api - PHP classes for the Echo Nest API
Summary: Review Request: php-echonest-api - PHP classes for the Echo Nest API
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-01-09 01:34 UTC by Randy Barlow
Modified: 2017-02-05 20:20 UTC (History)
3 users (show)

Fixed In Version: php-echonest-api-0-0.3.20131228git.662d62a7.fc26
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-02-02 20:51:14 UTC
Type: ---
fedora: fedora-review+


Attachments (Terms of Use)
phpci.log (7.66 KB, text/plain)
2017-01-16 16:42 UTC, Remi Collet
no flags Details
review.txt (8.89 KB, text/plain)
2017-01-16 16:43 UTC, Remi Collet
no flags Details

Description Randy Barlow 2017-01-09 01:34:53 UTC
Spec URL: https://bowlofeggs.fedorapeople.org/php-echonest-api.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-echonest-api-0-0.1.git.662d62a7.fc26.src.rpm
Description: A simple, Object Oriented API wrapper for the EchoNest Api written with PHP5.
This library is modeled after the php-github-api library built by ornicar.
Fedora Account System Username: bowlofeggs

Comment 1 Remi Collet 2017-01-09 09:54:28 UTC
Quick notes

> Requires:      php >= 5.2.0

No, it doesn't requires this (this is mod_php, which pull apache)
=> Use php(language)

> %global commit 662d62a7

=> use full reference

> %setup -q -n %{name}-662d62a7df1247515572bf517e14b795714e0824

=> Use previous macro

From phpcompatinfo report
Extensions Analysis

    Extension Matches REF      EXT min/Max PHP min/Max PHP all 
    Core              Core     5.1.0       5.1.0               
    curl              curl     5.1.3       5.1.3               
    date              date     4.0.0       4.0.0               
    json              json     5.2.0       5.2.0               
    spl               spl      5.1.2       5.1.2               
    standard          standard 4.0.7       4.0.7               
    xml               xml      4.0.0       4.0.0               
    Total [7]                              5.2.0               

=> So you need to requires php-curl, php-date, php-json, php-spl and php-xml

Seems to be PSR-0 compliant, so a PSR-0 tree is /usr/share/php/EchoNest
(but really not a problem, as we don't rely on this anymore, rather on autoloaders)

Nice to have a simple autoload.php (see tests/bootstrap for content), which can be maintained downstream if some day upstream drop it, and thus maintain compatibility.

Also use phpunit --boostrap=%{buildroot}/usr/share....
So "installed" code will be tested instead of code from sources tree.

Not registered on packagist (so probably no very commonly used)


** This looks like a dead project, no commit since 2013, no release. **

Do you really need this and want to maintain it ?
(btw, test suite passes with PHP 5.6, 7.0 and 7.1)

Comment 2 Randy Barlow 2017-01-15 01:27:13 UTC
Thanks for the notes Remi! I'm sure it's obvious to you that this is my first PHP package, so I really don't know much about what I'm doing. I used to code in PHP, but I haven't done that since 2004 and my has it changed since then (and how much I've forgotten anyway!) ☺ Responses inline.

(In reply to Remi Collet from comment #1)
> Quick notes
> 
> > Requires:      php >= 5.2.0
> 
> No, it doesn't requires this (this is mod_php, which pull apache)
> => Use php(language)

Corrected.

> > %global commit 662d62a7
> 
> => use full reference

Fixed.

> > %setup -q -n %{name}-662d62a7df1247515572bf517e14b795714e0824
> 
> => Use previous macro

Fixed.

> => So you need to requires php-curl, php-date, php-json, php-spl and php-xml

Fixed.

> Nice to have a simple autoload.php (see tests/bootstrap for content), which
> can be maintained downstream if some day upstream drop it, and thus maintain
> compatibility.

I copied the one from tests/bootstrap and adjusted it to match what I thought made sense. I'd love your input on what I did there, since I've not done PHP in so long.

> Also use phpunit --boostrap=%{buildroot}/usr/share....
> So "installed" code will be tested instead of code from sources tree.

Fixed.

> ** This looks like a dead project, no commit since 2013, no release. **
> 
> Do you really need this and want to maintain it ?
> (btw, test suite passes with PHP 5.6, 7.0 and 7.1)

So my goal here is to get Ampache packaged for Fedora. I've been working off of Ampache's composer.json to find its dependencies, and that's how I knew I needed this package:

https://github.com/ampache/ampache/blob/develop/composer.json

What do you think?

Here are some new files:

Spec URL: https://bowlofeggs.fedorapeople.org/php-echonest-api.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-echonest-api-0-0.2.git.662d62a7.fc26.src.rpm

Comment 3 Randy Barlow 2017-01-16 05:37:42 UTC
Remi,

I've put together a wiki page that charts the missing dependencies that Ampache will need. It seems that we have 5 other dependencies that have been explicitly abandoned upstream:

https://fedoraproject.org/wiki/Ampache

What do you think about that? It seems that the Ampache project is aware of these dependencies, and does have plans to migrate to supported upstreams. Can we package these abandoned dependencies until Ampache switches, and then retire the packages once they are no longer needed?

Comment 4 Remi Collet 2017-01-16 16:24:32 UTC
> What do you think about that? It seems that the Ampache project is aware of these dependencies, and does have plans to migrate to supported upstreams. Can we package these abandoned dependencies until Ampache switches, and then retire the packages once they are no longer needed?

As discussed on devel ML, yes, it's ok.

Comment 5 Remi Collet 2017-01-16 16:42:17 UTC
Created attachment 1241327 [details]
phpci.log

phpCompatInfo version 5.0.3 DB version 1.16.0 built Dec 16 2016 06:59:17 CET static analyze results

Comment 6 Remi Collet 2017-01-16 16:43:18 UTC
Created attachment 1241328 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1411188
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, PHP, Shell-api

Comment 7 Remi Collet 2017-01-16 16:44:22 UTC
Blockers:

[!]: Requires correct, justified where necessary.
	PHP virtual provides for extensions (curl, spl, ...) doesn't have version,
	so you should not use version in requires (doesn't make sense for most of ext.)
	Only php(language) should have a version.

[!]: Package complies to the Packaging Guidelines
	See https://fedoraproject.org/wiki/Packaging:Versioning
	So Release should be 0.<seq. number>.20131228git%{short_commit}%{?dist}

[!]: Rpmlint is run on all installed packages.
	Drop excution right on all php scripts

Comment 8 Randy Barlow 2017-01-21 18:23:22 UTC
Thanks Remi! Here are updated files with those fixes:

Spec URL: https://bowlofeggs.fedorapeople.org/php-echonest-api.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-echonest-api-0-0.3.20131228git.662d62a7.fc26.src.rpm

Comment 9 Remi Collet 2017-01-22 06:07:33 UTC
-Release:    0.2.git.%{short_commit}%{?dist}
+Release:    0.3.20131228git.%{short_commit}%{?dist}

[x]: Package complies to the Packaging Guidelines

-Requires:      php-curl >= 5.2.0
-Requires:      php-date >= 5.2.0
-Requires:      php-json >= 5.2.0
-Requires:      php-spl >= 5.2.0
-Requires:      php-xml >= 5.2.0
+Requires:      php-curl
+Requires:      php-date
+Requires:      php-json
+Requires:      php-spl
+Requires:      php-xml

[x]: Requires correct, justified where necessary.

+# https://github.com/Afterster/php-echonest-api/pull/1
+find . -name "*.php" | xargs chmod 0644

[x]: Rpmlint is run on all installed packages.


No more blocker

=== APPROVED ===

Comment 10 Gwyn Ciesla 2017-01-23 14:17:49 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/php-echonest-api

Comment 11 Fedora Update System 2017-01-24 09:04:39 UTC
php-echonest-api-0-0.3.20131228git.662d62a7.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-2eb370c36d

Comment 12 Fedora Update System 2017-01-24 09:05:16 UTC
php-echonest-api-0-0.3.20131228git.662d62a7.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-5a4124a65b

Comment 13 Fedora Update System 2017-01-25 01:23:05 UTC
php-echonest-api-0-0.3.20131228git.662d62a7.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-5a4124a65b

Comment 14 Fedora Update System 2017-01-28 19:19:52 UTC
php-echonest-api-0-0.3.20131228git.662d62a7.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-2eb370c36d

Comment 15 Fedora Update System 2017-02-02 20:51:14 UTC
php-echonest-api-0-0.3.20131228git.662d62a7.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2017-02-05 20:20:36 UTC
php-echonest-api-0-0.3.20131228git.662d62a7.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.