Bug 959926 - Review Request: libyui - GUI-abstraction library
Summary: Review Request: libyui - GUI-abstraction library
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/libyui/libyui/
Whiteboard:
Depends On:
Blocks: 960198 960199 960200 960201 963339
TreeView+ depends on / blocked
 
Reported: 2013-05-06 09:03 UTC by Björn 'besser82' Esser
Modified: 2014-03-18 11:47 UTC (History)
1 user (show)

Fixed In Version: libyui-3.0.5-1.fc19
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-05-16 12:50:19 UTC
Type: ---
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
one basic way to toggle the yui-ui dependency (710 bytes, patch)
2013-05-15 17:43 UTC, Michael Schwendt
no flags Details | Diff

Description Björn 'besser82' Esser 2013-05-06 09:03:27 UTC
Spec URL: http://besser82.fedorapeople.org/libyui/SPECS/libyui.spec
SRPM URL: http://besser82.fedorapeople.org/libyui/SRPMS/libyui-3.0.1-1.fc18.src.rpm

Description: This is the user interface engine that provides the abstraction from graphical user interfaces (Qt, Gtk) and text based user interfaces (ncurses). Originally developed for YaST, libyui can now be used independently of YaST for generic (C++) applications.

The SRPM successfully builds on koji.
rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=5334190
F20: http://koji.fedoraproject.org/koji/taskinfo?taskID=5334220
F19: http://koji.fedoraproject.org/koji/taskinfo?taskID=5334213
F18: http://koji.fedoraproject.org/koji/taskinfo?taskID=5334223

Fedora Account System Username: besser82

Hello!

This is my first package I want to contribute to Fedora, so I need a sponsor. I am in good contact to upstream, esp. to the lead-maintainer and have direct-push access to the libyui github-repos, too.

BR,
  Björn Esser

Comment 1 Björn 'besser82' Esser 2013-05-09 08:42:11 UTC
Updated to V3.0.3

You can find SPEC/SRPM here: http://besser82.fedorapeople.org/pkg/libyui/

Or clone my git-repo from besser82@fedorapeople.org:public_git/libyui.git and checkout the libyui branch.

BR,
  Björn Esser

Comment 2 Michael Schwendt 2013-05-12 11:44:32 UTC
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

Comment 3 Björn 'besser82' Esser 2013-05-12 14:45:12 UTC
Spec URL: http://besser82.fedorapeople.org/pkg/libyui/libyui.spec
SRPM URL: http://besser82.fedorapeople.org/pkg/libyui/libyui-3.0.3-1.fc18.src.rpm

Thanks for your review, Michael!

Uploaded fixed spec and new SRPM to location above.

Please review again.

Cheers,
  Björn.

Comment 4 Michael Schwendt 2013-05-12 20:40:01 UTC
Almost. ;)


* You've missed using  %cmake  instead of defining all the *FLAGS environment variables yourself.


* Some reviewers demand that package submitters increase the "Release" value for every update and add a %changelog entry, also during review: 

  https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes

It's good practice and makes running rpmdev-diff and rpmdiff more convenient.


* As why you've requested a "hardened build", not sure:
https://fedoraproject.org/wiki/Packaging:Guidelines#PIE


* Unowned directories fun:

libyui has been changed to not include /usr/share/doc/libyui-3.0.3/ -- that directory is "unowned" now:

  $ rpmls -p libyui-3.0.3-1.fc19.x86_64.rpm|grep ^d
  drwxr-xr-x  /usr/lib64/yui
  drwxr-xr-x  /usr/share/libyui
  drwxr-xr-x  /usr/share/libyui/theme

libyui-devel either needs to "Requires: cmake" (only for directory ownership) OR include a %dir entry for /usr/lib64/cmake/ (the latter is permitted):
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

If you choose to include the cmake directory in your package, be especially careful that the file permissions are exactly the same as in the cmake package.


libyui-doc has been changed to not include the "html" directory:

  $ rpmls -p libyui-doc-3.0.3-1.fc19.noarch.rpm|grep ^d
  drwxr-xr-x  /usr/share/doc/libyui-3.0.3/html/search


The following  patch shall fix the directory issues. Btw, it's okay to include full directory trees as you had done before. Example:

1)
  %{_includedir}/yui/

2)
  %{_includedir}/yui

3)  
  %dir %{_includedir}/yui
  %{_includedir}/yui/*
 
Those three are equivalent. The Wiki _tries_ to explain it.


--- libyui.spec.orig	2013-05-12 22:24:36.700740024 +0200
+++ libyui.spec	2013-05-12 22:26:51.794385741 +0200
@@ -118,6 +118,7 @@
 %dir %{_libdir}/yui
 %dir %{_datadir}/%{name}
 %dir %{_datadir}/%{name}/theme
+%dir %{_defaultdocdir}/%{name}-%{version}
 %doc %{_defaultdocdir}/%{name}-%{version}/C*
 
 
@@ -126,6 +127,7 @@
 %dir %{_includedir}/yui
 %{_includedir}/yui/*
 %{_libdir}/pkgconfig/%{name}.pc
+%dir %{_libdir}/cmake
 %dir %{_libdir}/cmake/%{name}
 %{_libdir}/cmake/%{name}/*
 %dir %{_datadir}/%{name}/buildtools
@@ -133,7 +135,9 @@
 
 
 %files doc
-%doc %{_defaultdocdir}/%{name}-%{version}/*/*
+%dir %{_defaultdocdir}/%{name}-%{version}
+%doc %{_defaultdocdir}/%{name}-%{version}/examples/
+%doc %{_defaultdocdir}/%{name}-%{version}/html/
 
 
 %changelog

Comment 5 Björn 'besser82' Esser 2013-05-13 17:34:58 UTC
Spec URL: http://besser82.fedorapeople.org/pkg/libyui/libyui.spec
SRPM URL: http://besser82.fedorapeople.org/pkg/libyui/libyui-3.0.3-2.fc18.src.rpm

Updated the package, see %changelog below.

Now the build rpms should own all required dirs:

drwxr-xr-x  /usr/include/yui
drwxr-xr-x  /usr/lib64/cmake/libyui
drwxr-xr-x  /usr/lib64/yui
drwxr-xr-x  /usr/share/doc/libyui-3.0.3
drwxr-xr-x  /usr/share/doc/libyui-3.0.3/examples
drwxr-xr-x  /usr/share/doc/libyui-3.0.3/html
drwxr-xr-x  /usr/share/doc/libyui-3.0.3/html/search
drwxr-xr-x  /usr/share/libyui
drwxr-xr-x  /usr/share/libyui/buildtools
drwxr-xr-x  /usr/share/libyui/theme

/usr/lib64/cmake/ -> -devel Requires: cmake (needed for libyui*-builds anyways)

%changelog
* Mon May 13 2013 Björn Esser <bjoern.esser@gmail.com> - 3.0.3-2
- fixup as suggested in https://bugzilla.redhat.com/show_bug.cgi?id=959926#c2
- add Patch0 to obey conventions about the compiler flags set in the system
  rpm configuration. See:
  https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
- fixup as suggested in https://bugzilla.redhat.com/show_bug.cgi?id=959926#c4
- build a hardened version just in case
- add Requires: %{libsuffix}-ui, because %{name} without UI-plugins is as
  useful as a car without gas and tires.
- add -devel Provides: %{libsuffix}-ui to provide a FAKE %{libsuffix}-ui to
  satisfy dependencies during rpmbuild of the UI-plugins and made sure this
  is known by documenting this in %{name}-devel description.
- add -devel Requires: cmake to solve the /usr/lib*/cmake/ ownership-problem,
  which is needed for libyui*-builds anyways.

* Mon May 06 2013 Björn Esser <bjoern.esser@gmail.com> - 3.0.3-1
- Initial RPM release.

Comment 6 Michael Schwendt 2013-05-14 15:31:26 UTC
* Minor typo: s/pakage/package/  in the new -devel %description.


* About the PIE build, I'm not convinced that a GUI framework should enable those build options (gtk3 and Qt don't do that either), but it's not a blocker.


* The following is an example of macro-madness: ;-p

  Patch0:         lib%{libsuffix}_honor_compiler_flags.patch

Files (such as these Patch files) typically have a fixed file name, especially since the files get checked into git. Using macros here doesn't make anything more convenient.


* It's good that you've made the yui-ui dependency use %_isa, but the idea
is somewhat half-baked. Let's see:

1) It's a circular dependency, which can become troublesome. 

Package libyui-$PLUGIN requires libyui/libyui-devel to build, and libyui requires any libyui-$PLUGIN to even install - or else no UI implementation would be available.

You cannot build a first libyui-$PLUGIN, because no plugin package
is available to satisfy the yui-ui dep in the libyui package when populating the buildroot. Therefore you would need to bootstrap your builds:

  - build a first libyui _without_ the yui-ui Requires
  - build a first libyui-$PLUGIN
  - rebuild libyui _with_ the yui-ui Req added back

And the consequences are that you would need repeat that procedure whenever you
want to build an incompatible libyui. Assume you've built an upgrade from
libyui.so.5 to libyui.so.6. Any available plugin package would still need
the old libyui.so.5, so there would be a broken dependency in the buildroot
upon trying to (re)build the plugin packages.

2) It makes no sense to provide a "fake" yui-ui in libyui-devel. *Any* such provider could be used by a dependency resolver to satisfy a requirement found in another package. So, if libyui requires yui-ui, installing libyui-devel
would satisfy the dependency _without_ providing a UI. Not just theoretically,
it's real. This is what happens during a test-installation:

# yum list libyui\* --enablerepo=plague-needsign
Loaded plugins: langpacks, refresh-packagekit
Available Packages
libyui.x86_64                          3.0.3-2.fc19              plague-needsign
libyui-devel.x86_64                    3.0.3-2.fc19              plague-needsign
libyui-doc.noarch                      3.0.3-2.fc19              plague-needsign
libyui-ncurses.x86_64                  2.43.8-1.fc19             plague-needsign
libyui-ncurses-devel.x86_64            2.43.8-1.fc19             plague-needsign
libyui-ncurses-doc.noarch              2.43.8-1.fc19             plague-needsign

# yum list libyui\* --enablerepo=plague-needsign
Loaded plugins: langpacks, refresh-packagekit
Available Packages
libyui.x86_64                          3.0.3-2.fc19              plague-needsign
libyui-devel.x86_64                    3.0.3-2.fc19              plague-needsign
libyui-doc.noarch                      3.0.3-2.fc19              plague-needsign
libyui-ncurses.x86_64                  2.43.8-1.fc19             plague-needsign
libyui-ncurses-devel.x86_64            2.43.8-1.fc19             plague-needsign
libyui-ncurses-doc.noarch              2.43.8-1.fc19             plague-needsign
[root@localhost ~]# yum install libyui --enablerepo=plague-needsign
Loaded plugins: langpacks, refresh-packagekit
plague-needsign                                          | 2.9 kB     00:00 !!! 
Resolving Dependencies
--> Running transaction check
---> Package libyui.x86_64 0:3.0.3-2.fc19 will be installed
--> Processing Dependency: yui-ui(x86-64) for package: libyui-3.0.3-2.fc19.x86_64
--> Running transaction check
---> Package libyui-devel.x86_64 0:3.0.3-2.fc19 will be installed
--> Finished Dependency Resolution

Dependencies Resolved

================================================================================
 Package            Arch         Version            Repository             Size
================================================================================
Installing:
 libyui             x86_64       3.0.3-2.fc19       plague-needsign       192 k
Installing for dependencies:
 libyui-devel       x86_64       3.0.3-2.fc19       plague-needsign       102 k

Transaction Summary
================================================================================
Install  1 Package (+1 Dependent package)

Total download size: 294 k
Installed size: 1.2 M
Is this ok [y/N]: n
Exiting on user command

Comment 7 Björn 'besser82' Esser 2013-05-14 15:53:58 UTC
Spec URL: http://besser82.fedorapeople.org/pkg/libyui/libyui.spec
SRPM URL: http://besser82.fedorapeople.org/pkg/libyui/libyui-3.0.3-3.fc18.src.rpm

Fixed up critical stuff.

%changelog
* Tue May 14 2013 Björn Esser <bjoern.esser@gmail.com> - 3.0.3-3
- removed macro from Patch0.
- fixed typo -> s/pakage/package/
- removed Provides/Requires: yui_ui
<snip>

Comment 8 Björn 'besser82' Esser 2013-05-15 11:39:11 UTC
Spec URL: http://besser82.fedorapeople.org/pkg/libyui/libyui.spec
SRPM URL: http://besser82.fedorapeople.org/pkg/libyui/libyui-3.0.4-1.fc18.src.rpm

New upstream version. Thanks for your review.

%changelog
* Wed May 15 2013 Björn Esser <bjoern.esser@gmail.com> - 3.0.4-1
- new upstream version
- add Patch1 to skip generation of pdf-docs if doxygen-latex is installed.
<snip>

Comment 9 Michael Schwendt 2013-05-15 17:43:18 UTC
Created attachment 748422 [details]
one basic way to toggle the yui-ui dependency

Okay, currently the yui-ui circular dep is missing, but you've said you will think about ways how to add it. Beyond that:

APPROVED

Comment 10 Björn 'besser82' Esser 2013-05-15 18:32:42 UTC
New Package SCM Request
=======================
Package Name: libyui
Short Description: GUI-abstraction library
Owners: besser82
Branches: f19

Comment 11 Gwyn Ciesla 2013-05-15 18:57:47 UTC
Git done (by process-git-requests).

Comment 12 Fedora Update System 2013-05-15 20:51:27 UTC
libyui-3.0.4-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/libyui-3.0.4-1.fc19

Comment 13 Björn 'besser82' Esser 2013-05-16 09:56:33 UTC
Package Change Request
======================
Package Name: libyui
New Branches: f18
Owners: besser82

Comment 14 Gwyn Ciesla 2013-05-16 12:20:30 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2013-05-16 13:54:34 UTC
libyui-3.0.5-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/libyui-3.0.5-1.fc19

Comment 16 Fedora Update System 2013-05-16 13:55:43 UTC
libyui-3.0.5-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libyui-3.0.5-1.fc18

Comment 17 Fedora Update System 2013-05-26 02:55:05 UTC
libyui-3.0.5-1.fc18 has been pushed to the Fedora 18 stable repository.

Comment 18 Fedora Update System 2013-05-26 03:37:10 UTC
libyui-3.0.5-1.fc19 has been pushed to the Fedora 19 stable repository.

Comment 19 Björn 'besser82' Esser 2014-03-18 11:42:21 UTC
Package Change Request
======================
Package Name: libyui
New Branches: el6
Owners: besser82

Want to build on el6, too.

Comment 20 Gwyn Ciesla 2014-03-18 11:47:47 UTC
Git done (by process-git-requests).


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