Bug 2445809

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.