Bug 567053 - Review Request: uzbl - Lightweight WebKit browser following the UNIX philosophy
Summary: Review Request: uzbl - Lightweight WebKit browser following the UNIX philosophy
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-02-21 09:44 UTC by Daiki Ueno
Modified: 2010-04-14 01:33 UTC (History)
5 users (show)

Fixed In Version: uzbl-0-0.9.20100221gitabbffe5c3.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-03-26 04:08:47 UTC
mtasaka: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Daiki Ueno 2010-02-21 09:44:24 UTC
Spec URL: http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl.spec
SRPM URL: http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl-0-0.1.20100221gitabbffe5c3.src.rpm
Description:
Uzbl is a lightweight web browser based on WebKit/Gtk+.  Uzbl follows
the UNIX philosophy - "Write programs that do one thing and do it
well. Write programs to work together. Write programs to handle text
streams, because that is a universal interface."

Comment 1 Mamoru TASAKA 2010-02-22 17:11:31 UTC
Some notes:

* Epoch
  - Please don't introduce Epoch

* Make build log more verbose
  - Build log messages like:
-------------------------------------------------------------
    38  + make -j4
    39  COMPILING src/callbacks.c
    40  COMPILING src/events.c
    41  COMPILING src/inspector.c
    42  COMPILING src/uzbl-core.c
    43  ... done.
-------------------------------------------------------------
    is not useful. We cannot check what is actually happening here,
    especially we cannot check if Fedora specific compilation flags
    are correctly honored or not:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags

    Please build log more verbose so that we can check if compiler
    flags are correctly honored.
    ! Note
      As far as I checked this package, actually Fedora specific compilation
      flags are currently not correctly honored, so this needs fixing.

* Build failure
  - Your srpm does not build on F-13:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=2005910
    This is because F-13 changed the options to pass to linker:
    http://lists.fedoraproject.org/pipermail/devel/2010-February/130519.html
    https://fedoraproject.org/wiki/UnderstandingDSOLinkChange

    You can check this by passing "--no-added" option to linker
    (for this package, you can do by doing $ make CC="gcc -Wl,--no-added")

* %makeinstall
  - Please avoid using this unless impossible:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

* Macros
  - Please use rpmmacros. /usr should be %{_prefix}
    https://fedoraproject.org/wiki/Packaging/RPMMacros

* Desktop files
  - GUI application should install proper desktop files:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

* Directories for document files
  - Currently this package installs document files under:
    - /usr/share/doc/uzbl-0
    - /usr/share/uzbl/docs
    - /usr/share/uzbl/examples
    Please consider to unify these (especially "AUTHORS" or so are installed
    both under the first two directories, so these should be fixed.

Comment 2 Mamoru TASAKA 2010-03-04 15:17:02 UTC
Ueno-san, would you update this bug?

Comment 3 Daiki Ueno 2010-03-07 15:11:53 UTC
Thanks for the review.  The issues should be fixed in -0.4:
http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl-0-0.4.20100221gitabbffe5c3.src.rpm

Comment 4 Mamoru TASAKA 2010-03-07 18:15:42 UTC
For 0.4:

* build process
  - From build.log:
-------------------------------------------------------------------------
    36  Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.yQ4cHw
    43  + make -j8
    54  cc -o uzbl-core  callbacks.o  events.o  inspector.o  uzbl-core.o -pthread -lwebkit-1.0 -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lpangoft2-1.0 -lgdk_pixbuf-2.0 -lpangocairo-1.0 -lcairo -lpango-1.0 -lfreetype -lfontconfig -lsoup-2.4 -lgio-2.0 -lgobject-2.0 -lgmodule-2.0 -lgthread-2.0 -lrt -lglib-2.0 -lX11   -pthread 
    56  Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.axM2dB
    69  + make DESTDIR=/builddir/build/BUILDROOT/uzbl-0-0.4.20100221gitabbffe5c3.i386 PREFIX=/usr install
    76  cc -o uzbl-core  callbacks.o  events.o  inspector.o  uzbl-core.o -pthread -lwebkit-1.0 -lgtk-x11-2.0 -lgdk-x11-2.0 -latk-1.0 -lpangoft2-1.0 -lgdk_pixbuf-2.0 -lpangocairo-1.0 -lcairo -lpango-1.0 -lfreetype -lfontconfig -lsoup-2.4 -lgio-2.0 -lgobject-2.0 -lgmodule-2.0 -lgthread-2.0 -lrt -lglib-2.0 -lX11   -pthread 
-------------------------------------------------------------------------
    So actually uzbl is compiled twice, once in %build and once
    in %install. It seems that Makefile does not check if the needed
    binaries are already rebuilt or not.
    So either
    - fix Makefile to prevent duplicate compilation
    - or just call "make" once (in %install, as binaries need to be installed
      anyway)

* Desktop file
  - Installed desktop file contains:
-------------------------------------------------------------------------
     5  Exec=uzbl
     6  Icon=uzbl
-------------------------------------------------------------------------
    However none of these are installed.

* "Example" files
  - Well, now I tried "uzbl-browser", and it complains:
-------------------------------------------------------------------------
$ env LANG=C uzbl-browser ; echo $?
cp: cannot stat `/usr/share/uzbl/examples/config/config': No such file or directory
Could not copy default config to /home/tasaka1/.config/uzbl/config
3
-------------------------------------------------------------------------
   I think the directory name "examples" is really confusing if
   this directory is actually needed on runtime, however it seems that
   this directory cannot be moved.

Comment 5 Daiki Ueno 2010-03-13 10:39:42 UTC
Thanks for the review.  The issues should be fixed in -0.5:
http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl-0-0.5.20100221gitabbffe5c3.src.rpm

On "examples" path, I ended up with keeping the default location as the upstream expects.

Comment 6 Mamoru TASAKA 2010-03-15 16:24:48 UTC
For -0.5:

Almost okay

* Timestamps
  https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
  - When using "cp" or "install" commands, please also add "-p" option
    to keep timestamps on installed files.

* Scriptlets for icon cache
  - Please follow below:
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

* Directory ownership issue
  https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership
  https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes
  - Currently the following directories themselves are not owned
    by any packages.
----------------------------------------------------------------
%{_datadir}/%{name}/
----------------------------------------------------------------

Comment 7 Daiki Ueno 2010-03-16 05:12:13 UTC
Thanks.  I've just uploaded -0.6 with fixes for them.

http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl-0-0.6.20100221gitabbffe5c3.src.rpm

Comment 8 Mamoru TASAKA 2010-03-17 07:02:32 UTC
For -0.6:
Almost okay.

* Directory ownership issue
  
  - Your spec file contains:
-------------------------------------------------------
    66  %files
    70  %{_datadir}/%{name}/
    71  %{_datadir}/%{name}/*
-------------------------------------------------------
    Now build.log shows warnings like:
-------------------------------------------------------
   111  warning: File listed twice: /usr/share/uzbl/examples
   112  warning: File listed twice: /usr/share/uzbl/examples/config
   113  warning: File listed twice: /usr/share/uzbl/examples/config/config
   114  warning: File listed twice: /usr/share/uzbl/examples/config/cookies
   115  warning: File listed twice: /usr/share/uzbl/examples/data
....
....
-------------------------------------------------------
    Note that as
    https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes
    shows, "%{_datadir}/%{name}/" list in %files contains the directory
    %{_datadir}/%{name} _and_ all directories/files/etc under this directory.

Comment 9 Daiki Ueno 2010-03-18 10:34:44 UTC
Ah, I misread the explanation on the Wiki :-)  Just put -0.7:
http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl-0-0.7.20100221gitabbffe5c3.src.rpm

Comment 10 Mamoru TASAKA 2010-03-18 15:54:46 UTC
Sorry, one more issue
* %define -> %global
  - Now we prefer to use %global instead of %define
    https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

Please fix this when importing this package into Fedora CVS.
----------------------------------------------------------
    This package (uzbl) is APPROVED by mtasaka
----------------------------------------------------------

Comment 11 Daiki Ueno 2010-03-24 05:12:24 UTC
Thanks.  Done in -0.8:
http://www.unixuser.org/~ueno/software/fedora/uzbl/uzbl-0-0.8.20100221gitabbffe5c3.src.rpm

I'll do CVS admin request shortly.

Comment 12 Daiki Ueno 2010-03-24 05:13:43 UTC
New Package CVS Request
=======================
Package Name: uzbl
Short Description: Lightweight WebKit browser following the UNIX philosophy
Owners: ueno
Branches: F-13 F-12 F-11

Comment 13 Dennis Gilmore 2010-03-25 20:59:14 UTC
CVS Done

Comment 14 Fedora Update System 2010-03-26 03:39:04 UTC
uzbl-0-0.9.20100221gitabbffe5c3.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/uzbl-0-0.9.20100221gitabbffe5c3.fc13

Comment 15 Fedora Update System 2010-03-26 03:40:38 UTC
uzbl-0-0.9.20100221gitabbffe5c3.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/uzbl-0-0.9.20100221gitabbffe5c3.fc12

Comment 16 Mamoru TASAKA 2010-03-26 04:08:47 UTC
Closing (maybe F-11 webkitgtk is too old for this package to compile)

Comment 17 Daiki Ueno 2010-03-26 12:07:47 UTC
Yes, I should have tested F-11 build before importing the package to CVS.
Can I remove the F-11 branch (or is it OK to leave the CVS tree for now)?

Comment 18 Mamoru TASAKA 2010-03-26 14:28:52 UTC
You can just leave F-11 branch as it is.

Comment 19 Fedora Update System 2010-04-14 01:33:17 UTC
uzbl-0-0.9.20100221gitabbffe5c3.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.


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