Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 225744 - Merge Review: fbset
Summary: Merge Review: fbset
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 18:35 UTC by Nobody's working on this, feel free to take it
Modified: 2009-02-10 09:58 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-02-10 09:58:20 UTC
adrian: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 18:35:19 UTC
Fedora Merge Review: fbset

http://cvs.fedora.redhat.com/viewcvs/devel/fbset/
Initial Owner: jnovy@redhat.com

Comment 1 Adrian Reber 2007-06-29 12:27:16 UTC
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 14:29:33 UTC
Zdenek Prikryl is the owner according the PackageDB.  Added as cc.

Comment 3 Zdenek Prikryl 2009-02-03 09:09:38 UTC
(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 13:52:40 UTC
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 09:29:42 UTC
(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 09:58:20 UTC
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.