Bug 1747552 - Review Request: libdfp - Decimal Floating Point library
Summary: Review Request: libdfp - Decimal Floating Point library
Keywords:
Status: POST
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Robert-André Mauchin
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1737946
TreeView+ depends on / blocked
 
Reported: 2019-08-30 19:35 UTC by Tulio Magno Quites Machado Filho
Modified: 2020-01-13 21:46 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
zebob.m: fedora-review+


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
IBM Linux Technology Center 181451 None None None 2019-09-12 15:10:05 UTC

Description Tulio Magno Quites Machado Filho 2019-08-30 19:35:55 UTC
Spec URL: https://pagure.io/libdfp/raw/master/f/libdfp.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tuliom/libdfp/srpm-builds/01008957/libdfp-1.0.14-3.fc30.src.rpm
Description: The "Decimal Floating Point C Library" is an implementation of ISO/IEC Technical report  "ISO/IEC TR 24732" which describes the C-Language library routines necessary to provide the C library runtime support for decimal
floating point data types introduced in IEEE 754-2008, namely _Decimal32, _Decimal64, and _Decimal128.
Fedora Account System Username: tuliom

This is my first package, so I need a sponsor.
Stefan Liebler (Fedora account stliibm) and me are maintainers of libdfp upstream and we'd like to co-maintain libdfp on Fedora.
We would like to enable the build on ppc64le and s390x.

We have successful builds in Copr: https://copr.fedorainfracloud.org/coprs/tuliom/libdfp/build/1008957/

The current spec is based on the latest version available on Enterprise Linux 7.
We updated the version, added support for make check and added more details.

Comment 1 Artur Iwicki 2019-09-03 08:19:26 UTC
>Group:		System Environment/Libraries
>BuildRoot:	%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>[...]
>Group:		Development/Libraries
The Group and BuildRoot tags are not used in Fedora.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

>%install
>rm -rf $RPM_BUILD_ROOT
>[...]
>%clean
>rm -rf $RPM_BUILD_ROOT
Don't remove the buildroot during %install. The %clean section is not needed.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

>%install
>make install install_root=$RPM_BUILD_ROOT
You can probably use %{make_install} here.

>cd Build
>[...]
>cd ..
Maybe use pushd / popd instead?

You should also include the COPYING.txt file in %files (and mark it as %license).

Comment 2 stli 2019-09-04 15:05:46 UTC
I've adjusted the spec file according to your comments.
But I can't use make_install as it would setup DESTDIR instead of install_root.

You'll find the current spec-file here:
https://pagure.io/fork/stliibm/libdfp/blob/stli-20190904/f/libdfp.spec
I've also created a pull-request: https://pagure.io/libdfp/pull-request/1

I've also performed some scratch builds for ppc64le and s390x on:
-F30: https://koji.fedoraproject.org/koji/taskinfo?taskID=37456479
-F31: https://koji.fedoraproject.org/koji/taskinfo?taskID=37456559
-epel8: https://koji.fedoraproject.org/koji/taskinfo?taskID=37456581

They are all green despite of epel8 on s390x where I've got test-fails:
FAIL: test-cast-to-overflow
FAIL: test-cast-to-underflow
=> These tests fails due to a missing patch in gcc (see https://github.com/libdfp/libdfp/issues/71#issuecomment-420614177). The patch was backported to gcc 8.3.

According to the logs for the epel8 build, gcc-8.2.1-3.5.el8.src.rpm was used.
Will epel8 use a newer gcc-8.3.* as soon as such a newer version is available in RHEL 8?

Comment 3 Robert-André Mauchin 2019-09-22 20:48:13 UTC
- Use %global, not %define:

%global cpu_variants power6

 - Missing isa:

Requires:	%{name}%{?_isa} = %{version}-%{release}

 - It seems it should be:

%prep
%autosetup -p1 -n %{name}-%{version}

 - make %{?_smp_mflags} → %make_build

 - make install install_root=%{buildroot} → %make_install

 - Not needed: %defattr(-,root,root,-)

 - Use %ldconfig_scriptlets instead of:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

 - In order to avoid unintentional soname bumps, we forbid the globbing of the major soname version, be more specific instead:

%{_libdir}/*.so.*

 - # Install COPYING.txt to _docdir.
Patch3: libdfp-license.patch

Why? COPYING.txt should not go to _docdir but be installed to licensedir with:

%license COPYING.txt

The file will be copied to the right location by rpm.


> Will epel8 use a newer gcc-8.3.* as soon as such a newer version is available in RHEL 8?

I have no idea how RHEL work, but EPEL8 will use it soon after it is available in RHEL.

Comment 5 Robert-André Mauchin 2019-10-09 15:58:46 UTC
You haven't addressed the follozwing: 

 - Missing isa in %package devel 

Requires:	%{name}%{?_isa} = %{version}-%{release}


 - It seems it should be:

%prep
%autosetup -p1 -n %{name}-%{version}


 - Not needed: %defattr(-,root,root,-) in %files devel

Comment 6 Tulio Magno Quites Machado Filho 2019-10-09 17:07:10 UTC
(In reply to Robert-André Mauchin from comment #5)
> You haven't addressed the follozwing: 
> 
>  - Missing isa in %package devel 
> 
> Requires:	%{name}%{?_isa} = %{version}-%{release}

I can't find this line:
$ git grep -En '^Requires:'
libdfp.spec:41:Requires:        %{name} = %{version}-%{release}

What am I missing?

>  - It seems it should be:
> 
> %prep
> %autosetup -p1 -n %{name}-%{version}
> 
> 
>  - Not needed: %defattr(-,root,root,-) in %files devel

Oops.  Sorry.  I fixed both issues now.
I decided to drop -n completely because the current package from upstream conforms to the %{name}-%{version} as expected by RPM.  Is that OK?

Spec URL: https://pagure.io/libdfp/raw/master/f/libdfp.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tuliom/libdfp/fedora-31-ppc64le/01051662-libdfp/libdfp-1.0.14-6.fc31.src.rpm

Results of the tests are available at:
https://copr.fedorainfracloud.org/coprs/tuliom/libdfp/build/1051662/

Comment 7 Robert-André Mauchin 2019-10-09 17:37:33 UTC
(In reply to Tulio Magno Quites Machado Filho from comment #6)
> (In reply to Robert-André Mauchin from comment #5)
> > You haven't addressed the follozwing: 
> > 
> >  - Missing isa in %package devel 
> > 
> > Requires:	%{name}%{?_isa} = %{version}-%{release}
> 
> I can't find this line:
> $ git grep -En '^Requires:'
> libdfp.spec:41:Requires:        %{name} = %{version}-%{release}
> 
> What am I missing?

Line 41 Add %{?_isa} after %{name}

> 
> >  - It seems it should be:
> > 
> > %prep
> > %autosetup -p1 -n %{name}-%{version}
> > 
> > 
> >  - Not needed: %defattr(-,root,root,-) in %files devel
> 
> Oops.  Sorry.  I fixed both issues now.
> I decided to drop -n completely because the current package from upstream
> conforms to the %{name}-%{version} as expected by RPM.  Is that OK?
> 

It's good.

Comment 8 Tulio Magno Quites Machado Filho 2019-10-09 19:22:02 UTC
(In reply to Robert-André Mauchin from comment #7)
> Line 41 Add %{?_isa} after %{name}

Oh! It's the opposite.
Done.  Thanks for your patience.

Spec URL: https://pagure.io/libdfp/raw/master/f/libdfp.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/tuliom/libdfp/fedora-31-ppc64le/01051723-libdfp/libdfp-1.0.14-7.fc31.src.rpm

Results of the tests are available at:
https://copr.fedorainfracloud.org/coprs/tuliom/libdfp/build/1051723/

Comment 9 Robert-André Mauchin 2019-10-10 14:25:31 UTC
Package approved.

You still need a sponsor. You also need to first sign the CLA on https://admin.fedoraproject.org/accounts
Also your Bugzilla email should be the same as your FAS account email.

See https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

and https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Ensure_your_package_is_suitable

Comment 10 Tulio Magno Quites Machado Filho 2020-01-13 20:25:03 UTC
I'm now in the packager group, but:

$ fedpkg request-repo libdfp 1747552
Could not execute request_repo: The Bugzilla bug's review was approved over 60 days ago

Robert, could you re-approve it, please?

I apologize...

Comment 11 Robert-André Mauchin 2020-01-13 20:30:42 UTC
No worry, flag refreshed.

Comment 12 Gwyn Ciesla 2020-01-13 21:46:28 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libdfp


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