https://bugzilla.redhat.com/show_bug.cgi?id=1982635 --- Comment #3 from Christopher Engelhard <ce@xxxxxxx> --- Thanks for the review. I'm working on the test issue and will set NEEDINFO once I have a new build including all fixes. (In reply to Otto Urpelainen from comment #1) > 2. > From fedora-review: > Note: Directories without known owners: /usr/share/php/League > This is a MUST item in the guidelines, > so it must be fixed. > I am not sure how vendor directory ownership is usually handed in php > packages. > I suppose all packages from League could share the ownership? > Then this would do it: %dir %{_datadir}/php/League > Reference: > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_file_and_directory_ownership Yes. Other packages in the \League namespace already own /usr/share/php/League, but since this one doesn't depend on any of them (unlike e.g. php-league-uri) it should own it as well. Will fix. > > > # Please preserve the changelog entries > I do not understand this request in combination with %autochangelog Once the package is actually build, rpmautospec will replace the macro with the actually (build from git) changelog. Hence, anybody messing around with a specfile they got from a source package should still preserve that changelog if possible. > 3. > Guidelines say > "The composer.json file is not used, > and should be installed as %doc > as it provides useful information about the package and its dependencies.". > I am not certain how to interpret this statement. > Since it says should, I do not think it is a blocking issue. > However, it clearly recommends installing the file as %doc > instead of dropping it completely > like is done here. > Perhaps it could be added as %doc as suggested? This is an oversight on my part. The composer file is useful (even if you don't use composer) because it e.g. tells the user which namespaces the PHP classes are in, where to find further documentation etc. I'll include it. > 4. > > : No tests implemented I'll include the tests by using a git checkout instead of the prepare tarball, as discussed here https://bugzilla.redhat.com/show_bug.cgi?id=1982616 (In reply to Otto Urpelainen from comment #2) > 1. > A release is packaged, but it downloaded by commit id rather than tag or > GitHub release. Nothing wrong with that, I am just curious why it is done > like that here? Because people most often look for releases etc. via composer/packagist.org, and that references packages via commit id: https://packagist.org/packages/beberlei/assert Packaging it this way a) makes life easier because all composer packages look very similar and b) ensures that no matter what upstream does in their git repo, the package people get via composer is the same they get via the repos. > 2. > > Autoloader: /usr/share/php/League/UriInterfaces/autoload.php > I am surprised that this information is important enough > to be added to description. > Why? Because you need to know where the autoloader is in order to actually use this package in a project, and its location standardized. No searchpath, unfortunately. > Also, there would be a slight readability gain > from formatting this differently, > so it would not look so much like a specfile tag. Good point, I hadn't noticed that. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
