Spec URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02329601-php-beberlei-assert/php-beberlei-assert.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02329601-php-beberlei-assert/php-beberlei-assert-3.3.1-1.fc35.src.rpm Description: A simple php library which contains assertions and guard methods for input validation (not filtering!) in business-model, libraries and application low-level code. The library can be used to implement pre-/post-conditions on input data. Since this package is set up to use rpmautospec, rpmlint/fedora-review will complain about missing dist tags & macros in changelog. These are spurious errors/warnings. This package is available from the lcts/nextcloud Copr, so you can also test it using 'fedora-review --copr-build 2329601' Fedora Account System Username: lcts
I have reviewed this package.Two findings: 1. > License: BSD That is what LICENSE and composer.json files say. Source files claim to be under MIT and then refer to nonexistent file LICENSE.txt. Even though any possible interpretation is allowed in Fedora, the license is unclear. Upstream must be contacted about this unclarity so that LICENSE file and source file headers can be made to agree. However, since it is quite clear that BSD 2-Clause is the intended license, I do consider it necessary to wait for a fix before this package can be added to fedora. 2. > : No tests implemented Same comment as for php-leagure-uri-interfaces [1]: There are tests upstream, but they remove them from Git archives. I would like to seem them ran, but not sure how to make that happen. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1982635
Small notices - yes, running upstream test suite make sense - PSR-0 tree doesn't make sense anymore, nobody will rely on a single PSR-0 tree It is fine to use /usr/share/php/Assert, but this can raise conflicts, so good be a good idea to introduce a fake "vendor" directory ex: /usr/share/php/beberlei/Assert - PSR-4 autoload is fine, but a classmap one can be faster (composer itself uses classmap for production optimized autoloader) - Documentation usually includes *.md files
Here is some possible changes +++ php-beberlei-assert.spec 2021-07-20 09:07:23.444053501 +0200 @@ -11,7 +11,7 @@ # PHP namespace and directory %global ns_project Assert -%global ns_dir %( echo "%{ns_project}" | sed 's|\\\\|\/|g' ) +%global ns_dir %{vendor}/%( echo "%{ns_project}" | sed 's|\\\\|\/|g' ) # Github %global gh_vendor beberlei @@ -20,7 +20,7 @@ %global scommit %(c=%{commit}; echo ${c:0:7}) # tests -%global with_tests 1 +%bcond_without tests #-- PREAMBLE ------------------------------------------------------------------# Name: php-%{vendor}-%{project} @@ -30,7 +30,8 @@ License: BSD URL: https://github.com/%{gh_vendor}/%{gh_project} -Source0: https://github.com/%{gh_vendor}/%{gh_project}/archive/%{commit}/%{gh_project}-%{version}-%{scommit}.tar.gz +Source0: %{name}-%{version}-%{scommit}.tgz +Source1: makesrc.sh BuildArch: noarch @@ -57,8 +58,18 @@ BuildRequires: %{_bindir}/php BuildRequires: php(language) >= 7.0 -%if 0%{?with_tests} -# for tests +%if %{with tests} +BuildRequires: phpunit9 +BuildRequires: php-simplexml +BuildRequires: php-mbstring +BuildRequires: php-ctype +BuildRequires: php-json +BuildRequires: php-intl +BuildRequires: php-date +BuildRequires: php-filter +BuildRequires: php-pcre +BuildRequires: php-reflection +BuildRequires: php-spl %endif # composer provides @@ -94,13 +105,9 @@ \Fedora\Autoloader\Autoload::addPsr4('%{ns_project}', __DIR__); // files & dependencies -\Fedora\Autoloader\Dependencies::required(array( +\Fedora\Autoloader\Dependencies::required([ __DIR__.'/functions.php', -)); - -\Fedora\Autoloader\Dependencies::optional(array( - // no optional dependencies -)); +]); EOF %check @@ -113,16 +120,29 @@ ? 0 : 1 ); " -%if 0%{?with_tests} +%if %{with tests} : Run tests -: No tests implemented +mkdir vendor +cat << 'EOF' | tee vendor/autoload.php +<?php +require_once '%{buildroot}%{_datadir}/php/%{ns_dir}/autoload.php'; +\Fedora\Autoloader\Autoload::addPsr4('%{ns_project}\\Tests', dirname(__DIR__).'/tests/Assert/Tests'); +\Fedora\Autoloader\Dependencies::required([ + '%{_datadir}/php/Yoast/PHPUnitPolyfills/autoload.php', + dirname(__DIR__).'/tests/Assert/Tests/Fixtures/functions.php', +]); +EOF + +phpunit9 --migrate-configuration +phpunit9 --verbose %endif #-- FILES ---------------------------------------------------------------------# %files %license LICENSE %doc composer.json -%{_datadir}/php/%{ns_dir} +%doc *md +%{_datadir}/php/%{vendor} #-- CHANGELOG -----------------------------------------------------------------# Using this, during the build: + phpunit9 --verbose PHPUnit 9.5.7 by Sebastian Bergmann and contributors. Runtime: PHP 7.4.21 Configuration: /dev/shm/extras/BUILD/assert-5e721d7e937ca3ba2cdec1e1adf195f9e5188372/phpunit.xml.dist ................................................................................................................................................................... 163 / 513 ( 31%) ................................................................................................................................................................... 326 / 513 ( 63%) ................................................................................................................................................................... 489 / 513 ( 95%) ........................ 513 / 513 (100%) Time: 00:00.026, Memory: 10.00 MB OK (513 tests, 904 assertions)
In above, missing the BuildRequires for yoast/phpunit-polyfills
Created attachment 1803525 [details] makesrc.sh
Thanks, Otto, Remi for the comments. > 1. > > License: BSD > That is what LICENSE and composer.json files say. > Source files claim to be under MIT > and then refer to nonexistent file LICENSE.txt. > Even though any possible interpretation is allowed in Fedora, > the license is unclear. > Upstream must be contacted about this unclarity > so that LICENSE file and source file headers can be made to agree. > However, since it is quite clear that BSD 2-Clause is the intended license, > I do consider it necessary to wait for a fix > before this package can be added to fedora. That makes sense. There is already an old issue regarding this upstream [1], which unfortunately hasn't been responded to. I'll see if I can get hold of the owner, they're still quite active on Github. > 2. > > : No tests implemented > Same comment as for php-leagure-uri-interfaces [1]: > There are tests upstream, but they remove them from Git archives. > I would like to seem them ran, but not sure how to make that happen. > [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1982635 Here I can use a local git checkout to create a source tarball that includes the tests (See for example here: [2]), as suggested by Remi. [1] https://github.com/beberlei/assert/issues/221 [2] https://src.fedoraproject.org/rpms/php-guzzlehttp-guzzle6/blob/rawhide/f/php-guzzlehttp-guzzle6.spec
Spec URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02364182-php-beberlei-assert/php-beberlei-assert.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/lcts/nextcloud/fedora-rawhide-x86_64/02364182-php-beberlei-assert/php-beberlei-assert-3.3.1-1.fc35.src.rpm Quick update: I've implemented the tests and moved the install directory to beberlei/Assert using Remi's patch (Thanks for that). Interestingly, some tests fail only on s390x! with 'IntlException: Constructor failed'. Must be somehow related to php-intl, but I haven't figured out why yet. I haven't gotten hold of the developer yet re: the license issue, I'll set NEEDINFO as soon as that is cleared up.
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.
Well, it has been a year, and it is the submitter that should perform the missing work. I'll clear fedora-review and unassign this review. In case there is progress, you can contact me again and I can finish the review.
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
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.