[Bug 890772] Review Request: tvlsim - Travel Market Simulator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=890772



--- Comment #20 from Felix Kaechele <felix@xxxxxxxxxxx> ---
A few notes before we can continue.

The package as linked here previously currently does not build. It looks like
the cmake macros need updating (e.g. %cmake instead of %cmake .; %cmake_build
and %cmake_install instead of %make_build and %make_install):
https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/.

Other observations, going through the spec file top to bottom:

1. L5: Please update to the latest upstream version.
2. L6: Consider using %autorelease for the Release tag:
https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_release_tag
3. L14-37: Consider expressing build dependencies in pkgconfig(libname) format
if the CMake backend uses pkg-config to find the library:
https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/
4. L30: Using file based BuildRequires is slow (due to needing to download and
traverse the file list metadata). The package that provides /usr/bin/epstopdf
also has an extra Provides: texlive-epstopdf which could be used instead.
5. L40-L69, L88-92: I recommend using https for URLs listed in the description,
where applicable
6. L96: the -n parameter could be removed from the %autosetup macro if you used
a different Source0 URL (using the GitHub anchor URL trick): Source0:       
%{url}/archive/refs/tags/%{version}.tar.gz#/%{name}-%{version}.tar.gz
7. L114 %check section can be removed if not used/not working.
8. L120,L141: COPYING file should be in %license not %doc
9. L122: %{_bindir}/simulate is a very generic binary name and may lead to
future conflicts (see
https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_potential_conflicting_files).
I couldn't find any other packages or projects online using this path, so this
is just an FYI.
10. L146: Consider using %autochangelog
(https://docs.pagure.org/fedora-infra.rpmautospec/autochangelog.html)

After these things are addressed I will give it a run with fedora-review (feel
free to run it against the package yourself as I will be going through all
warnings and errors it produces in the review).


-- 
You are receiving this mail because:
You are always notified about changes to this product and component
https://bugzilla.redhat.com/show_bug.cgi?id=890772

Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%20890772%23c20
_______________________________________________
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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux