Bug 2224783 - Review Request: dragonbox - Reference implementation of Dragonbox in C++
Summary: Review Request: dragonbox - Reference implementation of Dragonbox in C++
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/jk-jeon/dragonbox
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-07-22 18:47 UTC by Mattia Verga
Modified: 2023-09-04 08:16 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-09-04 08:16:34 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Mattia Verga 2023-07-22 18:47:17 UTC
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

Comment 1 Fedora Review Service 2023-07-22 18:52:02 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6204266
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2224783-dragonbox/fedora-rawhide-x86_64/06204266-dragonbox/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.

Comment 2 Felix Wang 2023-07-25 07:06:34 UTC
Taking this.

Comment 3 Felix Wang 2023-07-25 07:39:52 UTC
I only see an issue: static library should be avoided as possible. The remaining things LGTM.

Comment 4 Mattia Verga 2023-08-02 12:35:53 UTC
(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-libraries

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.

Comment 5 Mattia Verga 2023-08-26 08:23:01 UTC
So, do I need to adjust anything, or can the package be approved?

Comment 6 Zbigniew Jędrzejewski-Szmek 2023-09-01 13:19:06 UTC
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?

Comment 7 Mattia Verga 2023-09-01 13:32:21 UTC
(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.

Comment 8 Felix Wang 2023-09-02 03:39:28 UTC
> 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.

Comment 9 Mattia Verga 2023-09-02 07:41:37 UTC
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.

Comment 10 Zbigniew Jędrzejewski-Szmek 2023-09-02 09:26:13 UTC
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.

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

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

Comment 12 Fedora Admin user for bugzilla script actions 2023-09-02 11:52:15 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/dragonbox

Comment 13 Fedora Update System 2023-09-04 08:15:32 UTC
FEDORA-2023-8d303b9ee8 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-8d303b9ee8

Comment 14 Fedora Update System 2023-09-04 08:16:34 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.