Bug 2230617
| Summary: | Review Request: half - A C++ half-precision floating point type | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Tom Rix <trix> | ||||
| Component: | Package Review | Assignee: | Neal Gompa <ngompa13> | ||||
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
| Severity: | medium | Docs Contact: | |||||
| Priority: | unspecified | ||||||
| Version: | rawhide | CC: | jwakely, ngompa13, package-review | ||||
| Target Milestone: | --- | Flags: | ngompa13:
fedora-review+
|
||||
| Target Release: | --- | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Linux | ||||||
| URL: | http://sourceforge.net/projects/half | ||||||
| Whiteboard: | |||||||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |||||
| Doc Text: | Story Points: | --- | |||||
| Clone Of: | Environment: | ||||||
| Last Closed: | 2023-12-16 20:38:42 UTC | Type: | --- | ||||
| Regression: | --- | Mount Type: | --- | ||||
| Documentation: | --- | CRM: | |||||
| Verified Versions: | Category: | --- | |||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||
| Embargoed: | |||||||
| Bug Depends On: | |||||||
| Bug Blocks: | 1011110 | ||||||
| Attachments: |
|
||||||
|
Description
Tom Rix
2023-08-09 18:11:56 UTC
Copr build: https://copr.fedorainfracloud.org/coprs/build/6261352 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2230617-half/fedora-rawhide-x86_64/06261352-half/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. Taking this review. Initial spec review: > BuildArch: noarch This is not allowed on the main package, only on the devel subpackage. Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_header_only_libraries > rm -rf %{name}-%{version} > unzip -d %{name}-%{version} %{SOURCE0} > cd %{name}-%{version} This can be simplified to "%autosetup -c" > # change dos endings to unix > sed -i "s|\r||g" include/half.hpp > sed -i "s|\r||g" LICENSE.txt > sed -i "s|\r||g" README.txt I'm a bit wary of using sed for this when "dos2unix" is perfectly suitable here. Add "BuildRequires: dos2unix" and use the tool for this instead. > cd %{name}-%{version} This is no longer required, since %autosetup configures the %install phase correctly > mkdir -p %{buildroot}%{_docdir}/%{name}/ > install -m 644 LICENSE.txt %{buildroot}%{_docdir}/%{name}/ > install -m 644 README.txt %{buildroot}%{_docdir}/%{name}/ This can all be dropped, because we can manage this from %files... > %doc %{_docdir}/%{name}/README.txt > %license %{_docdir}/%{name}/LICENSE.txt This can be replaced with "%doc README.txt" and "%license LICENSE.txt". Spec URL: https://trix.fedorapeople.org/half.spec SRPM URL: https://trix.fedorapeople.org/half-2.2.0-2.fc40.src.rpm Thanks for the pointer on autosetup! Created attachment 1983920 [details]
The .spec file difference from Copr build 6261352 to 6314082
Copr build: https://copr.fedorainfracloud.org/coprs/build/6314082 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2230617-half/fedora-rawhide-x86_64/06314082-half/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. (In reply to Tom Rix from comment #0) > Spec URL: https://trix.fedorapeople.org/half.spec > SRPM URL: https://trix.fedorapeople.org/half-2.2.0-1.fc38.src.rpm > FAS: trix > > This is a C++ header-only library to provide an IEEE-754 conformant > half-precision floating point type along with corresponding arithmetic > operators, type conversions and common mathematical functions. GCC 13 already provides both std::float16_t and std::bfloat16_t as built-in types, with support in <cmath> (and <limits>, and <format>). Does this library take advantage of that if available? > It aims > for both efficiency and ease of use, trying to accurately mimic the > behaviour of the builtin floating point types at the best performance > possible. It automatically uses and provides C++11 features when > possible, but stays completely C++98-compatible when neccessary. This suggests it doesn't. Several ai packages use this file. The trade off is they all carry there own copies until if/when they switch over. I'm ok with dropping this as it would confuse a new, better solution. Sound ok ? I think the ideal solution would be for those packages to make use of this one (so they aren't each bundling it separately) and then for this package to be optimized to use the new built-in types when available (assuming they do actually perform better when implemented by the compiler+runtime). But since the new types are only available for C++23, it will probably be some time before those packages can actually benefit from them. So this package will probably make sense for several years. I didn't mean to express an objection to adding this package, I was just asking about the interaction with the new C++23 types, and raising awareness of them. Ok we can go ahead with the package. Thanks for the input. Any update on this package ? Review notes: * Package follows Fedora Packaging Guidelines * Package builds and installs * Package licensing is correctly handled * No serious issues from rpmlint PACKAGE APPROVED. The Pagure repository was created at https://src.fedoraproject.org/rpms/half |