https://bugzilla.redhat.com/show_bug.cgi?id=2335618 --- Comment #10 from Dominik 'Rathann' Mierzejewski <dominik@xxxxxxxxxxxxxx> --- (In reply to Matrew File from comment #7) > Notice that it currently doesn't work out of the box due to lacking EGL > fallback. This can be fixed now with some wrapper script hack but it's > mostly an upstream issue. > I'm inclined to wait for the upstream to pass the fix > (https://github.com/TypesettingTools/Aegisub/pull/375), but if you suggest > it can be patched in the packaging process. Your PR looks clean and works, so I'm in favour of patching this downstream. > Also, I'm currently still not sponsored. I would be happy if you could > review my status/contribution and offer a sponsorship. That's not a problem. Could you do a couple of reviews in the meantime and link them here? Some preliminary comments: 1. Package name should be lowercase (aegisub). Most other distributions (except Void), including existing RPM Fusion package for Fedora, use lowercase. Even upstream themselves use lowercase in many places. 2. Please use rm -rv here so that it's visible what gets deleted: find subprojects/ -mindepth 1 ! -path "subprojects/luabins*" -exec rm -rf {} + 3. Use rm -rv instead of -rf here: rm -rf packages/{osx_bundle,osx_dmg,win_installer} rm -rf tools/{osx-*,apply-manifest.py,*.ps1} rm -rf osx-bundle.sed 3. The comments in the %files section are redundant. %files -f %{altname}.lang # Application Desktop Entry %{_datadir}/applications/org.%{altname}.%{name}.desktop # metainfo %{_metainfodir}/org.%{altname}.%{name}.metainfo.xml # Executable %{_bindir}/%{altname}* # Plugins %{_datadir}/%{altname}/ # Application Icons %{_datadir}/icons/hicolor/*/apps/org.%{altname}.%{name}.* # License %license LICENCE 4. src/gl/glext.h seems to be an ancient version of /usr/include/GL/glext.h provided by mesa, but it doesn't seem to be used in the build. 5. There seems to be a test suite present in tests/ Could you get it to run in %check? A straightforward %check %meson_test seems to work here. 6. cajun-jsonapi seems to be bundled (libaegisub/{common,include}/cajun/*) but it's not declared with Provides: bundled(cajun-jsonapi). Bonus points if you can figure out the version. 7. BuildRequires lines should be sorted alphabetically. 8. The following BuildRequires: are not required to build, please drop them: git (only required for building from git repo and even then it should be git-core) intltool jansson-devel libcurl-devel (seems to be required only for the built-in update checker, which you should disable, anyway) libX11-devel lua-devel (only luajit is required) BuildRequires: libicu-devel is missing (though it's pulled in by boost-devel). BuildRequires: gettext is missing (though it's pulled in by fontconfig-devel indirectly). 9. meson finds dependencies using pkg-config, so they should be converted to BuildRequires: pkgcongfig(foo) style. 10. Release: 1%{?dist} You're using %autochangelog. Any reason why not %autorelease as well? 11. %global gituser TypesettingTools This seems to be used exactly once, so what's the point of the indirection. 12. # Main project is under BSD-3-Clause, some historical code is under ISC, and a few MIT License: BSD-3-Clause AND ISC AND MIT You need to enumerate which source files are under which license. It's OK to say e.g.: # most code is under BSD-3-Clause, except: # foo/bar.c: MIT # bar/baz.c: ISC 13. libaegisub/lua/modules/lpeg.{c,h} seems to be a bundled version of: https://www.inf.puc-rio.br/~roberto/lpeg/ That is packaged in Fedora already as lua-lpeg. Please check if it can be unbundled or at least declare it with Provides: bundled(lpeg) = <lpeg_version> -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2335618 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202335618%23c10 -- _______________________________________________ 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, report it: https://pagure.io/fedora-infrastructure/new_issue
