[Bug 959926] Review Request: libyui - GUI-abstraction library

Sun May 12 11:44:32 UTC 2013

Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=959926

Michael Schwendt <mschwendt at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|nobody at fedoraproject.org    |mschwendt at gmail.com
              Flags|                            |fedora-review?

--- Comment #2 from Michael Schwendt <mschwendt at gmail.com> ---
A first look at the spec file. Mostly minor issues (related to Fedora Packaging
Guidelines):

* rpmlint output is clean except for false positives about spelling errors
and shared-lib-calls-exit which is known and covered by a git pull request
already.

> BuildRequires:	boost-devel%{?_isa}
> BuildRequires:	cmake%{?_isa} >= 2.8
> BuildRequires:	doxygen%{?_isa}
> BuildRequires:	fdupes%{?_isa}
> BuildRequires:	gcc-c++%{?_isa}
> BuildRequires:	graphviz%{?_isa}
> BuildRequires:	pkgconfig%{?_isa}

Using %_isa in BuildRequires is more confusing than beneficial. First of all,
these "BuildRequires" become the src.rpm's "Requires":

  $ rpm -qpR libyui-3.0.3*.src.rpm
  boost-devel(x86-64)
  cmake(x86-64) >= 2.8
  doxygen(x86-64)
  fdupes(x86-64)
  gcc-c++(x86-64)
  graphviz(x86-64)
  pkgconfig(x86-64)
  rpmlib(FileDigests) <= 4.6.0-1
  rpmlib(CompressedFileNames) <= 3.0.4-1

And usually one publishes a single src.rpm in a common "source" repository
for all target architectures. The expanded %_isa value is the one from the
build environment the src.rpm has been built on. That may be wrong and
confusing. It could even be from a build machine with a completely different
architecture. For example, "boost-devel(x86-64)" is not available for i686,
where the requirement ought to be "boost-devel(x86-32)". It would be strictly
required to rebuild the src.rpm to fix its "Requires". Even if no conditional
arch-dependent build requirements are used in the spec file.

Conclusively: Not using %_isa in BuildRequires is fine and much more common,
too.

> BuildRequires:	gcc-c++%{?_isa}

gcc-c++ (as well as gcc and a few more) are expected to be available
in the "minimum build environment":

 https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires
  -> https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2

> %package devel
>
> Requires:	glibc-devel%{?_isa}
> Requires:	libstdc++-devel%{?_isa}

Same here. Except that are indirectly required by "gcc gcc-c++" already.

> Requires:	%{name}%{?_isa} = %{version}

At Fedora, the base package dependency is more strict and includes %{release},
too. This is because either the base package or the subpackage may be patched,
and a strict dependency enforces that they match eachother:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

> Summary:	Header files for %{name}

More accurate would be something like

  Summary: Files needed for developing with %{name}

since the package clearly contains more than just the headers. ;-)

> %package devel
> 
> URL:		https://github.com/%{name}/%{name}/

No need to duplicate the URL tag here. It is inherited from the base package
URL.

> %package doc
> 
> Group:		Development/Libraries
> 
> URL:		https://github.com/%{name}/%{name}/

Same here. Plus, group "Documentation" would be appropriate:
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

> %build
> CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS ;
> CXXFLAGS="${CXXFLAGS:-%optflags}" ; export CXXFLAGS ;
> FFLAGS="${FFLAGS:-%optflags%{?_fmoddir: -I%_fmoddir}}" ; export FFLAGS ;
> FCFLAGS="${FCFLAGS:-%optflags%{?_fmoddir: -I%_fmoddir}}" ; export FCFLAGS ;
> %{?__global_ldflags:LDFLAGS="${LDFLAGS:-%__global_ldflags}" ; export LDFLAGS ;}
> cmake .. \

Check out the %cmake macro which makes all this easier (rpm --eval %cmake):
http://fedoraproject.org/wiki/Packaging:Cmake

> %install
> cd build
> rm -rf %{buildroot}

It is emptied automatically already:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

> make DESTDIR=%{buildroot} install

Nowadays there's a  %make_install  macro for that.

> install -m0644 

Using also option -p can be helpful for packages, not just if they contain
old/ancient files:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

> %clean
> rm -rf %{buildroot}

https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean

> %dir %{_datadir}/%{name}
>
> --      Theme-Dir:                             /usr/share/libyui/theme

It would make sense to include this [empty] directory in this package, because
there is code in the lib that uses this path. A pkgconfig variable is defined
for it, too.

> %doc %dir %{_defaultdocdir}/%{name}-%{version}

It's a directory and never marked as %doc. Files are to be marked as %doc.

> %defattr(-,root,root)

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

> %files devel
> %doc %dir %{_defaultdocdir}/%{name}-%{version}
> %doc %{_defaultdocdir}/%{name}-%{version}/C*

https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files

Same for the -doc subpackage.

> %{_libdir}/cmake/%{name}

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

* Doing a test-build with a modified spec file using the  %cmake  macro,
build.log output gets more verbose and reveals that somewhere "-O3 -g3"
is being appended:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=SPCByHLopl&a=cc_unsubscribe