[Bug 2224783] New: Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Bug ID: 2224783
           Summary: Review Request: dragonbox - Reference implementation
                    of Dragonbox in C++
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

Spec URL: https://mattia.fedorapeople.org/dragonbox/dragonbox.spec
SRPM URL:
https://mattia.fedorapeople.org/dragonbox/dragonbox-1.1.3-1.fc39.src.rpm
Description: Dragonbox is a float-to-string conversion algorithm based on a
beautiful algorithm Schubfach, developed by Raffaello Giulietti in 2017-2018.
Dragonbox is further inspired by Grisu and Grisu-Exact.
Fedora Account System Username: mattia

Koji scratch build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=103754677

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Fedora Review Service  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://github.com/jk-jeon/
                   |                            |dragonbox

--- Comment #1 from Fedora Review Service  ---
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6204266
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...

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 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Felix Wang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review?
           Doc Type|---                         |If docs needed, set a value
                 CC|                            |[e-mail]

--- Comment #2 from Felix Wang  ---
Taking this.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

--- Comment #3 from Felix Wang  ---
I only see an issue: static library should be avoided as possible. The
remaining things LGTM.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

--- Comment #4 from Mattia Verga  ---
(In reply to Felix Wang from comment #3)
> I only see an issue: static library should be avoided as possible. The
> remaining things LGTM.

This is a header only library, so adheres to
https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-...

BTW, if you're willing to make a formal review, you should set the ticket
status to ASSIGNED and set yourself as the assignee.
Thanks.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Petr Pisar  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|[e-mail]    |[e-mail]
             Status|NEW                         |ASSIGNED

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Mattia Verga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |needinfo?(topazus@outlook.c
                   |                            |om)

--- Comment #5 from Mattia Verga  ---
So, do I need to adjust anything, or can the package be approved?

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Zbigniew Jędrzejewski-Szmek  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #6 from Zbigniew Jędrzejewski-Szmek  ---
Hmm, so what is in the .a file? If it is not needed for anything, maybe drop it
and make the -devel
package noarch?
> BTW, if you're willing to make a formal review, you should set the ticket status to ASSIGNED and set yourself as the assignee.

I see that Felix didn't actually assign this to himself, and it was assigned by
Petr instead.
Felix, do you still want to continue the review?

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

--- Comment #7 from Mattia Verga  ---
(In reply to Zbigniew Jędrzejewski-Szmek from comment #6)
> Hmm, so what is in the .a file? If it is not needed for anything, maybe drop
> it and make the -devel
> package noarch?

I'm not really expert of C, but here are an excerpt from the README:
"""
Although Dragonbox is intended for float-to-string conversion routines, the
actual string generation is not officially a part of the algorithm. Dragonbox
just outputs two integers (the decimal significand/exponent) that can be
consumed by a string generation procedure. The header file
include/dragonbox/dragonbox.h includes everything needed for this (it is
header-only). Nevertheless, a string generation procedure is included in the
library. There are two additional files needed for that:
include/dragonbox/dragonbox_to_chars.h and source/dragonbox_to_chars.cpp.
"""

If you think it isn't worth to distribute the .a file, the instructions say I
can use '-DDRAGONBOX_INSTALL_TO_CHARS=OFF' to disable building the library.
I'll have to check if LibreOffice build just requires the headers, though,
since this is the only reason to package dragonbox.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Felix Wang  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|fedora-review?              |
                   |needinfo?(topazus@outlook.c |
                   |om)                         |
           Assignee|[e-mail]         |[e-mail]

--- Comment #8 from Felix Wang  ---
> I see that Felix didn't actually assign this to himself, and it was assigned by Petr instead.
> Felix, do you still want to continue the review?

Sorry for the late response. I have been very busy with my graduation thesis
this month and do not have time and energy to spare on this, so I withdraw from
the package review assignment.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

--- Comment #9 from Mattia Verga  ---
Spec URL: https://mattia.fedorapeople.org/dragonbox/dragonbox.spec
SRPM URL:
https://mattia.fedorapeople.org/dragonbox/dragonbox-1.1.3-1.fc40.src.rpm

So, LibreOffice only needs the headers, therefore I've disabled the build of
the static library and made the package noarch.
Note that Fedora Packaging Guidelines says that only the subpackage should be
noarch, while the main package must be arch'ed so that tests are run. But this
package has no tests, so I see no point in wasting resources to build it
multiple times on all arches... I dropped a note in the specfile, if in future
tests are added (unlikely) I will move the noarch to the subpackage only.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Zbigniew Jędrzejewski-Szmek  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review+
           Assignee|[e-mail]    |[e-mail]
             Status|NEW                         |POST

--- Comment #10 from Zbigniew Jędrzejewski-Szmek  ---
With the latest changes, the review is trivial:

+ package name is OK
+ latest version
+ license is acceptable for Fedora (Apache-2.0 and change)
- license is specified correctly
  (see below)
+ builds and installs OK
+ BR/R/P look reasonable
> License: Apache-2.0 WITH LLVM-exception AND BSL-1.0

Shouldn't this be "OR" instead? Both files that are in the binary package
have the same header that says "Alternatively, the contents of this file may be
used under
the terms of the BSL".

Please add a comment how the license is derived.

I don't think this makes any difference in practice, so the
package is APPROVED.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

--- Comment #11 from Mattia Verga  ---
(In reply to Zbigniew Jędrzejewski-Szmek from comment #10)
> Shouldn't this be "OR" instead?

Yeah, right. I'll change upon import.
Thanks.

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

--- Comment #12 from Fedora Admin user for bugzilla script actions  ---
The Pagure repository was created at
https://src.fedoraproject.org/rpms/dragonbox

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |MODIFIED

--- Comment #13 from Fedora Update System  ---
FEDORA-2023-8d303b9ee8 has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2023-8d303b9ee8

[Bug 2224783] Review Request: dragonbox - Reference implementation of Dragonbox in C++

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

Fedora Update System  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|MODIFIED                    |CLOSED
         Resolution|---                         |ERRATA
        Last Closed|                            |2023-09-04 08:16:34

--- Comment #14 from Fedora Update System  ---
FEDORA-2023-8d303b9ee8 has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.