https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Ben Beasley <code@xxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(code@musicinmybra |needinfo?(riehecky@xxxxxxxx |in.net) |) --- Comment #3 from Ben Beasley <code@xxxxxxxxxxxxxxxxxx> --- This fails to build on either x86_64 or aarch64 for me, so here are some initial comments without the aid of fedora-review (and without digging into the source tree for now). The first is the cause of the build failure. ===== Issues ===== - It’s my understanding that the *-static virtual Provides for header-only libraries (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_header_only_libraries) should not be arched. Some packages providing them unfortunately violate this, which can lead to occasional build failures, but “json” is not one of them. Please change > BuildRequires: json-devel json-static%{?_isa} to > BuildRequires: json-devel json-static to resolve this. - Please explain/justify the makefile patch in a spec file comment, and either indicate that it is not suitable for upstream, or point out that you have offered it upstream. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_all_patches_should_have_an_upstream_bug_link_or_comment. - I don’t think you should add -std=c++0x to the build flags unless it’s entirely unavoidable. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags. You should try to build the C++ code with the same C++ standard that is the default for g++ in Fedora—I think this is currently equivalent to gnu++17. Consider that there is no ABI compatibility across different C++ standards, so when library packages in Fedora use different C++ flags from the distribution defaults, dependent packages can fail to link or, worse, crash at runtime. - The License field is incorrect; you must use one of the short identifiers from https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses. “Apache License 2.0” is written as “ASL 2.0”. The “very permissive license” on the MD5 implementation has a name; it is the RSA license. It is approved for Fedora (https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses, https://fedoraproject.org/wiki/Licensing/RSA). Since it’s not clear to me that the terms of the RSA license are encompassed by the Apache license, I think it should be included in the License field. See https://fedoraproject.org/wiki/Licensing:FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F and https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios. I think the License field should look like this. > # The entire source is ASL 2.0 except third_party/md5/, which is RSA > License: ASL 2.0 and RSA - This > %{__make} install DESTDIR=%{buildroot} PREFIX=%{_prefix} should be written as > %{make_install} PREFIX=%{_prefix} Importantly, the macro sets INSTALL="%{_bindir}/install -p" so that file mtimes are preserved. - You should include the shared library SONAME version in the file globs so you do not roll an update with an soversion bump by accident. (For Rawhide, these updates require advance notification and should include rebuilds of any dependent packages in a side tag—https://docs.fedoraproject.org/en-US/fesco/Updates_Policy/#_rawhide. For stable releases, these updates are allowed only in exceptional circumstances.) See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files, and https://src.fedoraproject.org/rpms/libpri/blob/rawhide/f/libpri.spec for an example. (You don’t have to put the soversion in a macro, but I find it convenient.) - The -devel package correctly requires the base package with a fully-versioned dependency, > Requires: %{srcname} = %{version}-%{release} but this should be arched as well: > Requires: %{srcname}%{?_isa} = %{version}-%{release} See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package. - You can drop > %license LICENSE from the %files section for the -devel package since it requires the base package, so the LICENSE file will always be installed. See https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#subpackage-licensing. - Assuming the tests do not have parallel-make bugs, you can and should write > %{__make} test as > %{make_build} test -- 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
