Spec URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10204738/glaze.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10204738/glaze-7.1.1-1.src.rpm Description: One of the fastest JSON libraries in the world. Glaze reads and writes from object memory, simplifying interfaces and offering incredible performance. Fedora Account System username: bby256 Needed for Hyprland version 0.46+. This review request is part of my effort to get the Hyprland ecosystem back in official Fedora repositories. The package+dependencies for Hyprland is currently retired/stale/missing.
Copr build: https://copr.fedorainfracloud.org/coprs/build/10204788 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2445809-glaze/fedora-rawhide-x86_64/10204788-glaze/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Maybe helpful to see https://bugzilla.redhat.com/show_bug.cgi?id=2366921
(In reply to Benson Muite from comment #2) > Maybe helpful to see https://bugzilla.redhat.com/show_bug.cgi?id=2366921 I know about that one. nightishaman has been inactive for months and I made this request with the fixes implemented to the spec file that is broken on the bug you linked.
Do update the links, thanks
Presumably it is not x86_64 only, right? (arch field)
Spec URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/fedora-rawhide-x86_64/10262637-glaze/glaze.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10262637/glaze-7.1.1-1.src.rpm Changed arch field. Updated links.
Thanks Did you try to unbundle xxhash (like in the earlier package review)? Also can you comment on the disabling of SIMD?
Spec URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/fedora-rawhide-x86_64/10263974-glaze/glaze.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10262637/glaze-7.1.1-1.src.rpm Unbundled xxhash with wrapper pointer method. Removed manual SIMD disable flag.
Copr build: https://copr.fedorainfracloud.org/coprs/build/10265671 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2445809-glaze/fedora-rawhide-x86_64/10265671-glaze/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Copr build: https://copr.fedorainfracloud.org/coprs/build/10265682 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2445809-glaze/fedora-rawhide-x86_64/10265682-glaze/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
*** Bug 2366921 has been marked as a duplicate of this bug. ***
I would like to take over the review of this package. If you are open to it, I would appreciate it if you could help review ly in return: https://bugzilla.redhat.com/show_bug.cgi?id=2459725
I tested the current package: SRPM build, rebuild, install, and a small CMake consumer test all succeeded. A few issues still need to be addressed before approval: 1. The spec comment explains MIT + bundled dragonbox, but I could not confirm the current BSD-2-Clause part from the sources. 2. since fast_float-devel is already packaged in Fedora, is there still a need to bundle fast_float here? https://src.fedoraproject.org/rpms/fast_float
Sure, sorry for being late. I will address the issues today.
Spec URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10412870/glaze.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10412870/glaze-7.1.1-1.src.rpm BSD-2-Clause issue addressed (Removed from spec) About fast_float: The bundled one in the package is namespace wrapped, making the system fast_float package incompatible.
Copr build: https://copr.fedorainfracloud.org/coprs/build/10412887 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2445809-glaze/fedora-rawhide-x86_64/10412887-glaze/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Hi, Bekir 1. Provides: bundled(fast_float) has the wrong version. The spec currently has: Provides: bundled(fast_float) = 6.1.1 but the bundled header says it is fast_float 8.0.2: // glaze-7.1.1/include/glaze/util/fast_float.hpp #define GLZ_FASTFLOAT_VERSION_MAJOR 8 #define GLZ_FASTFLOAT_VERSION_MINOR 0 #define GLZ_FASTFLOAT_VERSION_PATCH 2 2. The package is currently at 7.1.1, while upstream has released 7.5.0. It would be good to update to the latest release.
maybe you could try this patch to unbundle fast_float: in %prep, replace the bundled include/glaze/util/fast_float.hpp with a small wrapper around the system header: # Replace the bundled fast_float header with a wrapper pointing to the system version. cat > include/glaze/util/fast_float.hpp <<'EOF' #pragma once #include <fast_float/fast_float.h> namespace glz { namespace fast_float = ::fast_float; } EOF The spec also needs: Patch0: 0001-use-system-fast-float.patch BuildRequires: fast_float-devel Requires: fast_float-devel 0001-use-system-fast-float.patch: --- a/include/glaze/util/glaze_fast_float.hpp +++ b/include/glaze/util/glaze_fast_float.hpp @@ -42,7 +42,7 @@ uint64_t digit; if constexpr (null_terminated) { - while ((digit = glz::fast_float::digit_value(*p)) <= 9) { + while ((digit = uint64_t(*p - UC('0'))) <= 9) { // a multiplication by 10 is cheaper than an arbitrary integer // multiplication i = 10 * i + digit; // might overflow, we will handle the overflow later @@ -50,7 +50,7 @@ } } else { - while ((p != pend) && (digit = glz::fast_float::digit_value(*p)) <= 9) { + while ((p != pend) && (digit = uint64_t(*p - UC('0'))) <= 9) { // a multiplication by 10 is cheaper than an arbitrary integer // multiplication i = 10 * i + digit; // might overflow, we will handle the overflow later @@ -84,13 +84,13 @@ loop_parse_if_eight_digits(p, pend, i); if constexpr (null_terminated) { - while ((digit = glz::fast_float::digit_value(*p)) <= 9) { + while ((digit = uint64_t(*p - UC('0'))) <= 9) { ++p; i = i * 10 + digit; // in rare cases, this will overflow, but that's ok } } else { - while ((p != pend) && (digit = glz::fast_float::digit_value(*p)) <= 9) { + while ((p != pend) && (digit = uint64_t(*p - UC('0'))) <= 9) { ++p; i = i * 10 + digit; // in rare cases, this will overflow, but that's ok } @@ -118,7 +118,7 @@ else if (UC('+') == *p) { // '+' on exponent is allowed by C++17 20.19.3.(7.1) ++p; } - if ((digit = glz::fast_float::digit_value(*p)) > 9) { + if ((digit = uint64_t(*p - UC('0'))) > 9) { // Otherwise, we will be ignoring the 'e'. p = location_of_e; } @@ -128,7 +128,7 @@ exp_number = 10 * exp_number + digit; } ++p; - } while ((digit = glz::fast_float::digit_value(*p)) <= 9); + } while ((digit = uint64_t(*p - UC('0'))) <= 9); if (neg_exp) { exp_number = -exp_number; } @@ -148,7 +148,7 @@ else if ((p != pend) && (UC('+') == *p)) { // '+' on exponent is allowed by C++17 20.19.3.(7.1) ++p; } - if ((p == pend) || (digit = glz::fast_float::digit_value(*p)) > 9) { + if ((p == pend) || (digit = uint64_t(*p - UC('0'))) > 9) { // Otherwise, we will be ignoring the 'e'. p = location_of_e; } @@ -158,7 +158,7 @@ exp_number = 10 * exp_number + digit; } ++p; - } while ((p != pend) && (digit = glz::fast_float::digit_value(*p)) <= 9); + } while ((p != pend) && (digit = uint64_t(*p - UC('0'))) <= 9); if (neg_exp) { exp_number = -exp_number; }
Spec URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10415466/glaze.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10415466/glaze-7.5.0-1.src.rpm Patch applied to unbundle fast_float. Version bumped to 7.5.0.
Upstream has replaced dragonbox with zmij which is not in the system repositories. Now there is a bundled zmij.
1. %global debug_package %{nil} is acceptable here. Fedora Packaging Guidelines explicitly mention this case for header-only libraries whose base package remains arched while only noarch subpackages are built. However, the Debuginfo guidelines require an explanation in the spec whenever debuginfo generation is explicitly disabled. Please add a comment explaining this. https://src.fedoraproject.org/rpms/fast_float/blob/rawhide/f/fast_float.spec#_38 https://src.fedoraproject.org/rpms/xxhashct/blob/rawhide/f/xxhashct.spec#_24 2. The License comment/field still refers to dragonbox / Apache-2.0, but upstream 7.5.0 appears to have replaced dragonbox with zmij. Please update the License field and comment accordingly.
Spec URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10416107/glaze.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/bby256/hyprland-stack/srpm-builds/10416107/glaze-7.5.0-1.src.rpm Added comment. Fixed license mismatch.
Copr build: https://copr.fedorainfracloud.org/coprs/build/10416720 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2445809-glaze/fedora-rawhide-x86_64/10416720-glaze/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Created attachment 2139035 [details] The .spec file difference from Copr build 10416720 to 10416724
Copr build: https://copr.fedorainfracloud.org/coprs/build/10416724 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2445809-glaze/fedora-rawhide-x86_64/10416724-glaze/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Thank you for the updates. I have reviewed and everything looks good to me.
Thanks, can you also sponsor me to the packager group please?
Hi, Bekir Just to clarify, I’m a Fedora packager and can help with reviewing the package, but I do not have sponsor status. Therefore I’m not able to sponsor new packagers into the packager group. I can still continue with the package review and provide feedback, but you will need an existing Fedora packager sponsor for the sponsorship part.