Bug 225744 - Merge Review: fbset
Merge Review: fbset
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 13:35 EST by Nobody's working on this, feel free to take it
Modified: 2009-02-10 04:58 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-10 04:58:20 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
adrian: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 13:35:19 EST
Fedora Merge Review: fbset

http://cvs.fedora.redhat.com/viewcvs/devel/fbset/
Initial Owner: jnovy@redhat.com
Comment 1 Adrian Reber 2007-06-29 08:27:16 EDT
This looks pretty much like it can be closed. There are four little points we
should be changed:

* change buildroot to be guidelines compliant
* add comment why %{?_smp_mflags} cannot be used
* add comment why make install DESTDIR=%{buildroot} cannot be used
* add noreplace to the config (to silence rpmlint)

--- fbset.spec  18 Jan 2007 15:37:11 -0000      1.21
+++ fbset.spec  29 Jun 2007 12:24:40 -0000
@@ -10,7 +10,7 @@
 Patch0: fbset-2.1-makefile.patch
 Patch1: fbset-2.1-fixmode.patch
 Patch2: fbset-2.1-manfix.patch
-BuildRoot: %{_tmppath}/%{name}-root
+BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
 ExcludeArch: s390 s390x
 
 %description
@@ -27,11 +27,13 @@
 %patch2 -p1 -b .man
 
 %build
+# %{?_smp_mflags} breaks building
 make CFLAGS="$RPM_OPT_FLAGS"
 
 %install
 rm -rf ${RPM_BUILD_ROOT}
 
+# make install DESTDIR=%{buildroot} does not work here
 %makeinstall
 
 %clean
@@ -41,7 +43,7 @@
 %defattr(-,root,root)
 %{_sbindir}/*
 %{_mandir}/man[58]/*
-%config        %{_sysconfdir}/fb.modes
+%config(noreplace) %{_sysconfdir}/fb.modes
 
 %changelog
 * Thu Jan 18 2007 Jindrich Novy <jnovy@redhat.com> - 2.1-24


Comment 2 Fabian Affolter 2009-01-29 09:29:33 EST
Zdenek Prikryl is the owner according the PackageDB.  Added as cc.
Comment 3 Zdenek Prikryl 2009-02-03 04:09:38 EST
(In reply to comment #1)
> * change buildroot to be guidelines compliant

Fixed.

> * add comment why %{?_smp_mflags} cannot be used

fbset doesn't use autotool's stuff, it is written by hand. So, you have to use CFLAGS variable to pass additional options to the gcc.

> * add comment why make install DESTDIR=%{buildroot} cannot be used

This is a similar problem like above. In the makefile, there is no such variable. There is a $(PREFIX) variable which is set by %makeinstall

> * add noreplace to the config (to silence rpmlint)

Fixed.

I committed the new spec into cvs. Please, review it, so I can bump a new release.

Zdenek
Comment 4 Adrian Reber 2009-02-05 08:52:40 EST
There is something in the guidelines about %makeinstall:

http://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

The right thing, to conform with the guidelines would be to use:

make install sysconfdir=%{buildroot}%{_sysconfdir} sbindir=%{buildroot}%{_sbindir} mandir=%{buildroot}%{_mandir}

Although, that makes it pretty unreadable. I would say it is up to you which one you like better.

And I have seen this:

MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.

There is tracker bug for s390 at:
https://bugzilla.redhat.com/show_bug.cgi?id=467765

Please create a bug blocking that one and mention the bug near the ExcludeArch line.
Comment 5 Zdenek Prikryl 2009-02-10 04:29:42 EST
(In reply to comment #4)
> There is something in the guidelines about %makeinstall:
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used
> 

Since "make install DESTDIR=%{buildroot}" doesn't work and fbset doesn't use libtool or autotools stuff, I'd not remove the %makeinstall macro.

> And I have seen this:
> 
> MUST: If the package does not successfully compile, build or work on an
> architecture, then those architectures should be listed in the spec in
> ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
> bugzilla, describing the reason that the package does not compile/build/work on
> that architecture. The bug number MUST be placed in a comment, next to the
> corresponding ExcludeArch line.
> 

I created the bug and write ID to the spec file. Check cvs.
Comment 6 Adrian Reber 2009-02-10 04:58:20 EST
I am just mentioning the ExcludeArch bug here:

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

Rest looks good.

Source matches upstream.
Patches look good.

I am not 100% happy with the %makeinstall, but it seems that it creates no problems in this case. So:

APPROVED.

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