[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #25 from Ben Beasley --- (In reply to Pat Riehecky from comment #24) > If I've got the relevant permissions, I'll take a look when it is ready. > > I'll be out of the office for a bit over the holidays. Tag me in with a > needinfo and I'll be sure to get back to it. Thanks! Any packager can do a review. See https://docs.fedoraproject.org/en-US/package-maintainers/Package_Review_Process/#_reviewer for instructions. -- 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=1973682 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #24 from Pat Riehecky --- If I've got the relevant permissions, I'll take a look when it is ready. I'll be out of the office for a bit over the holidays. Tag me in with a needinfo and I'll be sure to get back to it. -- 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=1973682 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #23 from Ben Beasley --- (In reply to Pat Riehecky from comment #22) > The new version of jsonnet came out today with a dependency on rapidyaml. > Any success getting that packaged for Fedora? Some. I got https://src.fedoraproject.org/rpms/c4project and https://src.fedoraproject.org/rpms/c4core packaged as dependencies. Now I need a package review for c4fs, https://bugzilla.redhat.com/show_bug.cgi?id=2025359. Unfortunately, there aren’t enough people reviewing packages right now, so review requests are languishing for many months unless I go looking for review swaps. If you’d be willing to do the review for c4fs and, when it’s ready, for rapidyaml, then I can start working on rapidyaml again. -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=1973682 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #22 from Pat Riehecky --- The new version of jsonnet came out today with a dependency on rapidyaml. Any success getting that packaged for Fedora? -- 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=1973682 ___ package-review mailing list -- package-review@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Ben Beasley changed: What|Removed |Added Status|NEW |CLOSED Resolution|--- |RAWHIDE Last Closed||2021-06-25 18:34:48 -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #21 from Gwyn Ciesla --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/jsonnet -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682
Ben Beasley changed:
What|Removed |Added
Flags|fedora-review? |fedora-review+
--- Comment #20 from Ben Beasley ---
Package approved! Don’t let me forget to write you a pair of man pages.
Package Review
==
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
= Issues =
- The python3-jsonnet subpackage does not have to install the LICENSE file
separately since it depends on the -libs subpackage. (However, it is
permitted to do so.)
= MUST items =
C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
Note: Unversioned so-files in private %_libdir subdirectory (see
attachment). Verify they are not in ld path.
There is a Python extension module, correctly installed.
[x]: If your application is a C or C++ application you must list a
BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
Generic:
[x]: Package is licensed with an open-source compatible license and meets
other legal requirements as defined in the legal section of Packaging
Guidelines.
[x]: License field in the package spec file matches the actual license.
Note: Checking patched sources after %prep for licenses. Licenses
found: "Unknown or generated", "*No copyright* Apache License 2.0",
"Apache License 2.0", "*No copyright* Creative Commons Attribution 2.5
Generic License", "NTP License". 483 files have unknown license.
Detailed output of licensecheck in
/home/reviewer/1973682-jsonnet/licensecheck.txt
Note that the RSA license is incorrectly detected as “NTP License”
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
must be documented in the spec.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
Multiple bundled libraries, correctly removed in %prep, except md5
copylib which is correctly handled.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
(~1MB) or number of files.
Note: Documentation size is 20480 bytes in 2 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Pat Riehecky changed: What|Removed |Added Flags|needinfo?(riehe...@fnal.gov | |) | --- Comment #19 from Pat Riehecky --- Updated with the proposed feedback (I think I missed the source URL comment earlier): SPEC: https://download.copr.fedorainfracloud.org/results/jcpunk/jsonnet/fedora-34-x86_64/02296050-jsonnet/jsonnet.spec SRPM: https://download.copr.fedorainfracloud.org/results/jcpunk/jsonnet/fedora-34-x86_64/02296050-jsonnet/jsonnet-0.17.0-1.fc34.src.rpm -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #18 from Ben Beasley --- Created attachment 1793588 --> https://bugzilla.redhat.com/attachment.cgi?id=1793588=edit Candidate patch to stop overriding build flags Patch mentioned in latest review -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682
Ben Beasley changed:
What|Removed |Added
Flags|needinfo?(code@musicinmybra |needinfo?(riehe...@fnal.gov
|in.net) |)
--- Comment #17 from Ben Beasley ---
I’m not quite done picking at the details, but we’re getting very close.
Package Review
==
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
= Issues =
- Unless I am missing something, you should set -DBUILD_STATIC_LIBS:BOOL=OFF,
and remove the -static subpackage. From
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_static_libraries,
“In general, packagers SHOULD NOT ship static libraries.”
- The build system is still overriding the optimization flags (adding -O3) and
the C++ standard flags (forcing C++11) from the environment
(https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags).
I am attaching a patch that should correct this.
Note that linking C++ code compiled with different standard versions is not
reliable, as there is no ABI compatibility across different 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.
- I missed this before, but please split the shared libraries into a -libs
subpackage. See https://bugzilla.redhat.com/show_bug.cgi?id=1955394#c5 and
the following discussion for an explanation of how this helps with multilib
filtering.
Once you’ve done this, you’ll have to change
Requires: %{name}%{?_isa} = %{version}-%{release}
to
Requires: %{name}-libs%{?_isa} = %{version}-%{release}
in the -devel package, and add it in the base package.
You should also add the same fully-versioned dependency to the
python3-jsonnet subpackage; even though it automatically depends on the
shared library, we want the version and release to stay in sync within
subpackages built from the same source.
See
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package.
- As long as you are packaging the (currently un-built but partially usable)
documentation, you might as well throw in the examples/ to the -doc
subpackage—probably without the check.sh scripts.
= Notes (no change required) =
- I still think that
Source0:%{url}/archive/v%{version}/%{name}-%{version}.tar.gz
would result in a better archive name than the current
Source0:%{url}/archive/refs/tags/v%{version}.tar.gz
but you are not required to change it.
- I very much agree with your choice to remove the dependency on the base
package from the -doc subpackage; I was going to request that, but you beat
me to it.
- The patch to link the Python extension dynamically is great. I’m not sure I
would have taken the time to figure that out. Static linking across
subpackages of the same source RPM is not against the Fedora guidelines, but
it’s always nice to reduce duplication.
= MUST items =
C/C++:
[x]: Package does not contain kernel modules.
[!]: Package contains no static executables.
Static library (.a) is built and installed.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
Note: Unversioned so-files in private %_libdir subdirectory (see
attachment). Verify they are not in ld path.
There is a Python extension module, correctly installed.
[x]: If your application is a C or C++ application you must list a
BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
Generic:
[x]: Package is licensed with an open-source compatible license and meets
other legal requirements as defined in the legal section of Packaging
Guidelines.
[x]: License field in the package spec file matches the actual license.
Note: Checking patched sources after %prep for licenses. Licenses
found: "Unknown or generated", "*No copyright* Apache License 2.0",
"Apache License 2.0", "*No copyright* Creative Commons Attribution 2.5
Generic License", "NTP License". 483 files have unknown license.
Detailed output of licensecheck in
/home/reviewer/1973682-jsonnet/licensecheck.txt
Note that the RSA license is incorrectly detected as “NTP License”
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
must be documented in the spec.
[!]: %build honors applicable compiler flags or justifies otherwise.
See Issues.
[x]: Package contains no bundled
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Ben Beasley changed: What|Removed |Added Flags||needinfo?(code@musicinmybra ||in.net) -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Pat Riehecky changed: What|Removed |Added Flags|needinfo?(riehe...@fnal.gov | |) | --- Comment #14 from Pat Riehecky --- > - Man pages are always desired for command-line executables > (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages). > > Unfortunately, help2man doesn’t generate very satisfactory output. If you > are > willing to maintain a pair of hand-written downstream man pages in > groff_man(7) format, I can supply them—perhaps as a PR once the package is > approved. I'd be fine to maintain those. I'll confess I've not much success in making them SPEC: https://download.copr.fedorainfracloud.org/results/jcpunk/jsonnet/fedora-34-x86_64/02286250-jsonnet/jsonnet.spec SRPM: https://download.copr.fedorainfracloud.org/results/jcpunk/jsonnet/fedora-34-x86_64/02286250-jsonnet/jsonnet-0.17.0-1.fc34.src.rpm -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #12 from Ben Beasley --- Review request for debugbreak, another piece in the rapidyaml dependency tree: https://bugzilla.redhat.com/show_bug.cgi?id=1974821 -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682
Ben Beasley changed:
What|Removed |Added
Flags|needinfo?(code@musicinmybra |needinfo?(riehe...@fnal.gov
|in.net) |)
--- Comment #11 from Ben Beasley ---
We’re getting closer and closer!
Package Review
==
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
Issues:
===
- Development (unversioned) .so files in -devel subpackage, if present.
Note: Unversioned so-files directly in %_libdir.
See: https://docs.fedoraproject.org/en-US/packaging-
guidelines/#_devel_packages
The fedora-review text above covers this pretty well. This:
%{_libdir}/lib%{srcname}*.so
needs to be moved to the %files section for the -devel.
You might then want to change
Summary:%{summary} development headers
(which expands to “Diff JSON and JSON-like structures development headers”,
which is a little nonsensical anyway), to:
Summary:Development files for %{name}
- I haven’t looked into this:
jsonnet.aarch64: E: invalid-ldconfig-symlink /usr/lib64/libjsonnet++.so
libjsonnet++.so.0.17.0
jsonnet.aarch64: E: invalid-ldconfig-symlink /usr/lib64/libjsonnet.so
libjsonnet.so.0.17.0
but hopefully it’s related to the above. You can run “fedora-review -n
jsonnet” in a directory with the jsonnet spec file and source RPM (and as a
user belonging to the “mock” group) to find out for yourself if this is
resolved.
- While it’s not wrong,
%package -n %{srcname}-devel
would be much more simply written as
%package devel
and the same for the corresponding %description and files section. (Changing
this is not mandatory.)
- In fact, since you are not relying on %{pypi_source} (which uses %srcname or
%pypi_name if set), and the package base name is the same as the Python
metadata name and importable module name, you don’t have to set %srcname at
all. You could just drop
%global srcname jsonnet
and write
Name: jsonnet
and then use %{name} instead of %{srcname} everywhere else in the file. (The
%name macro is automatically set based on the Name field.) (Changing this is
not mandatory.)
- Here,
%if 0%{?fedora} && 0%{?fedora} == 34
the first conditional is redundant, as %fedora will always be set and nonzero
when it is set to 34. You could just write:
%if 0%{?fedora} == 34
(Changing this is not mandatory.)
- The Source0 url could be improved in two ways: you can use the %url macro to
write it more concisely, and (as a GitHub-specific thing) you can write it in
a form that gives you a nicer tarball name.
Source0:%{url}/archive/v%{version}/%{name}-%{version}.tar.gz
(Changing this is not mandatory.)
- Man pages are always desired for command-line executables
(https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages).
Unfortunately, help2man doesn’t generate very satisfactory output. If you are
willing to maintain a pair of hand-written downstream man pages in
groff_man(7) format, I can supply them—perhaps as a PR once the package is
approved.
- In general, it’s good to build the documentation if needed and include it in
a -doc subpackage.
In this case, there are some issues: it bundles CodeMirror, js-yaml, and
MathJax
(https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling), and
these include pre-compiled JavaScript
(https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_compilationminification)
and CSS
(https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/#_css).
Additionally, these include assets under various additional licenses.
It may be possible to deal with all of this and build the documentation
anyway, but I don’t want to hold up the package review on trying to do so. If
I have some time later, I’ll consider working on this and making a PR. For
now, I suggest removing the problematic assets in %prep:
# Remove bundled and pre-minified web assets
rm -rf doc/third_party
and leaving the documentation unpackaged. Note that if you do package it,
doc/_layouts/base.html says the documentation is licensed under Creative
Commons Attribution 2.5 (Fedora: CC-BY).
- You should add CONTRIBUTING to %doc, alongside README.md.
- Rpmlint reports:
jsonnet.src:48: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line
48)
Please fix this.
= MUST items =
C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #10 from Ben Beasley --- Review request for fast_float: https://bugzilla.redhat.com/show_bug.cgi?id=1974431 Reviewing your latest package is up next… -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Ben Beasley changed: What|Removed |Added Flags||needinfo?(code@musicinmybra ||in.net) --- Comment #9 from Ben Beasley --- Thanks! I’ll take a look at it. I appreciate your effort in figuring out the Right Way(TM) to package this. I’m almost ready to submit a review request for fast_float, which is a dependency for rapidyaml. So that will be at least one piece out of the way. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Pat Riehecky changed: What|Removed |Added Flags|needinfo?(riehe...@fnal.gov | |) | --- Comment #8 from Pat Riehecky --- Thanks for all the detailed feedback thus far! I'm planning to eventually do EPEL. I quick look at the EPEL branches shows they are providing the non-arched json-static so it looks like I just need a Fedora 34 specific check. I've got a new thingy built up: SPEC: https://download.copr.fedorainfracloud.org/results/jcpunk/jsonnet/fedora-34-aarch64/02285141-jsonnet/jsonnet.spec SRPM: https://download.copr.fedorainfracloud.org/results/jcpunk/jsonnet/fedora-34-aarch64/02285141-jsonnet/jsonnet-0.17.0-1.fc34.src.rpm -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 --- Comment #7 from Ben Beasley --- (In fact, it looks like that’s exactly what I did in https://src.fedoraproject.org/rpms/giada, and then forgot about it.) -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682
--- Comment #6 from Ben Beasley ---
> I'm showing current Fedora isn't providing the non-arched version of
> json-static:
Ugh, you’re right. The maintainer of json-static correctly added the non-arched
version in
https://src.fedoraproject.org/rpms/json/c/dcb2174e7bf406af13cbe40750909650fde153de
and subsequently removed the arched versions in
https://src.fedoraproject.org/rpms/json/c/c1b06b23466eb54347528bfa962941e6bedb78d7?branch=rawhide,
which is why I couldn’t build your spec on Rawhide for review.
However, these commits aren’t in the builds for Fedora 34 and older, so the
unarched dependency doesn’t work there.
(Follow-up: it looks like you found the same while I was writing this.)
Since the arched -static dependency can lead to unexpected build failures, I’d
suggest just doing something like this for now:
> BuildRequires: json-devel
> # Un-arched -static virtual Provides is in F35+ only
> # https://bugzilla.redhat.com/show_bug.cgi?id=1974376
> %if 0%{?fedora} != 33 && 0%{?fedora} != 34
> BuildRequires: json-static
> %endif
altering the conditional if you’re planning to support EPEL8. (Are you?)
--
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682
--- Comment #4 from Pat Riehecky ---
I'll try and get those changes in place.
>
>- 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.
I'm showing current Fedora isn't providing the non-arched version of
json-static:
$ rpm -q json-devel --provides |grep static
json-static(x86-64) = 3.9.1-2.fc34
--
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682
Ben Beasley changed:
What|Removed |Added
Flags|needinfo?(code@musicinmybra |needinfo?(riehe...@fnal.gov
|in.net) |)
--- Comment #3 from Ben Beasley ---
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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct:
[Bug 1973682] Review Request: jsonnet - A data templating language
https://bugzilla.redhat.com/show_bug.cgi?id=1973682 Ben Beasley changed: What|Removed |Added CC||c...@musicinmybrain.net Assignee|nob...@fedoraproject.org|c...@musicinmybrain.net Doc Type|--- |If docs needed, set a value Flags||fedora-review? ||needinfo?(code@musicinmybra ||in.net) --- Comment #1 from Ben Beasley --- Continuing from https://bugzilla.redhat.com/show_bug.cgi?id=1973322. Thanks! It might take me a day or two to actually get to this, but I’m claiming the review as a continuation of the previous Bugzilla. -- 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@lists.fedoraproject.org To unsubscribe send an email to package-review-le...@lists.fedoraproject.org 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@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
