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 454208 - Review Request: florence - Florence is an extensible scalable on-screen virtual keyboard for GNOME
Summary: Review Request: florence - Florence is an extensible scalable on-screen virt...
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2008-07-06 17:49 UTC by Simon
Modified: 2014-05-19 12:00 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-08-03 13:13:56 UTC
mtasaka: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)
florence-0.2.2-warnings.patch (deleted)
2008-07-26 20:25 UTC, Robert Scheck
no flags Details | Diff

Description Simon 2008-07-06 17:49:13 UTC
Spec URL:
Description: Florence is an extensible scalable virtual keyboard for GNOME. 
You need it if you can't use a real hardware keyboard, for 
example because you are disabled, your keyboard is broken or 
because you use a tablet PC, but you must be able to use a pointing 
device (as a mouse, a trackball or a touchscreen)

This is my secound package - I'm looking for a sponsor
my first package:

Comment 1 Simon 2008-07-06 19:48:23 UTC
I made a big mistake

please CANCEL review request

I have to work on it

Comment 2 manuel wolfshant 2008-07-06 20:21:59 UTC
Simon, if you are sure that you want this bug to be ignored, you can "close" it,
by selecting "Resolve bug" (available below, under "Bug Status change").
However if you plan to come back with a revised spec, just leave it as it is and
submit the new version when ready.

Comment 4 Mamoru TASAKA 2008-07-18 14:33:00 UTC
Interesting package.

Some random notes for 0.2.2-2:

* License
  - As far as I checked the whole codes, the license tag should
    be "GPLv2+ and GFDL".

  - The following lines:
export CFLAGS
    are redundant and should be removed (please check what
    %configure actually does by
    $ rpm --eval %configure )

* desktop-file-install usage
desktop-file-install --vendor="fedora" \
        --dir=$RPM_BUILD_ROOT%{_datadir}/applications \
rm -f $RPM_BUILD_ROOT%{_datadir}/applications/%{name}.desktop
  - You can use --delete-original option (please check
    $ desktop-file-install --help)

* GConf scriptlets
  - For GConf scriptlets, please refer to
    %post scriptlet is missing
  - Also please add proper "Requires(pre)" or so.
  - We regards GConf schemas files as _not_ a configuration file
    so please remove %config(noreplace) attribution on GConf schemas file
    (even if rpmlint warns about it)

* sed call on %post
  - Modifying %{_sysconfdir}/gconf/schemas/%{name}.schemas (using sed)
    must not be done on scriptlets but must be done before %install finishes.

* Directory ownership issue
  - Please make it sure that all directories created when installing this
    package are correctly owned by this package.
    For example, the directory %{_datadir}/%{name}/ is not owned by any package.

  ! NOTE
    When you write
    This contains the directory %{_datadir}/%{name} itself and all files/
    directories/etc under %{_datadir}/%{name}, while
%dir %{_datadir}/%{name}/
    contains the directory %{_datadir}/%{name} only.

* Documents
  - Please add the following files to %doc:

Comment 5 Mamoru TASAKA 2008-07-18 14:40:41 UTC
BTW the description "Florence is" is redundant for Summary.

Comment 6 Mamoru TASAKA 2008-07-18 15:33:10 UTC
One more thing
* Timestamps
  - Please consider to use
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
    to keep timestamps on installed files as much as possible. This method usually
    works for recent autotool based Makefiles.

Comment 8 Mamoru TASAKA 2008-07-25 18:50:08 UTC

* Scriptlets
  - Requires(preun): GConf is missing

* %configure
  - --prefix=/usr is unneeded. Please check again what %configure does
    by $ rpm --eval %configure

* Macros
  - Please use macros correctly
sed -i "s|@LAYOUTDIR@|/usr/share/florence|"
    /usr/share must be %{_datadir} (rpm expands this correctly)

* Directory ownership issue
  - This is not yet correctly fixed. For example the directory
    %{_datadir}/%{name}/ is not owned by any packages.

* Some issues from build.log
   637  GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL is set, not installing schemas
   638  ./../gconf-refresh
   639  kill: usage: kill [-s sigspec | -n signum | -sigspec] pid | jobspec ...
or kill -l [sigspec]
   640  Unable to send HUP signal to gconf daemon.
   641  Configuration will not be updated until reboot.
   642  Florence may not function correctly before reboot.
  - Calling "kill -HUP gconfd" command during build is not preferable.
    As this "gconf-refresh" script is not useful for building this
    package, please replace this script with some other command
    e.g. writing the following at %prep:
rm -f gconf-refresh
ln -sf /bin/true gconf-refresh

  - The other issue is
   259  keyboard.c:388: warning: implicit declaration of function
    Would you try to remove "implicit declaration of function' warning?

Comment 9 Robert Scheck 2008-07-26 18:02:46 UTC
(In reply to comment #8)
>   - The other issue is
> -------------------------------------------------------------------
>    259  keyboard.c:388: warning: implicit declaration of function
> 'layoutreader_readkeyboard'
> -------------------------------------------------------------------
>     Would you try to remove "implicit declaration of function' warning?
>     c.f.

Sorry, but fixing code has to be done by UPSTREAM and not by the maintainer 
which tries to get downstream hereby. It is a warning and fine. Maybe a bug
report should be opened on upstream side's tracking tool, but he shouldn't be
forced or bugged to fix the bugs/insanities of the upstream code. Otherwise
the kernel package review will never succeed, if we're going to be downstream
and upstream in Fedora of everything.

Comment 10 Mamoru TASAKA 2008-07-26 18:19:53 UTC
(In reply to comment #9)
> (In reply to comment #8)
> >   - The other issue is
> > -------------------------------------------------------------------
> >    259  keyboard.c:388: warning: implicit declaration of function
> > 'layoutreader_readkeyboard'
> > -------------------------------------------------------------------
> >     Would you try to remove "implicit declaration of function' warning?
> >     c.f.
> >
> Sorry, but fixing code has to be done by UPSTREAM and not by the maintainer 
> which tries to get downstream hereby. It is a warning and fine. 

While "I said would you 'try'?", however I must note:

GCC's 'warning'  means 'serious bug' on many cases...
Actually leaving the warnings of 'implicit declaration of function' sometimes causes
segv on the application (and this happened on my package!!) and 'leave it to
Fedora's maintainer won't fix it' can _never_ be Fedora's policy.

Comment 11 Mamoru TASAKA 2008-07-26 18:22:57 UTC

Comment 12 Robert Scheck 2008-07-26 20:25:29 UTC
Created attachment 312715 [details]

The attached patch should get rid about all compiler warnings for implicit 
declarations and unused variables. Please review and forward to upstream.

Comment 14 Mamoru TASAKA 2008-07-27 15:13:34 UTC
Well, for 0.2.2-4:

* Patch name
Patch0:         %{name}-%{version}-warnings.patch
  - For the names of patches, you usually want not to use macros
    (especially %version).
    It often happens that you don't have to modify patches even if
    the tarball is upgraded. However if you use %{version} for patches'
    names, you have to recreate patches again even in such cases.

* Modifying CFLAGS

CFLAGS="${RPM_OPT_FLAGS} -Werror-implicit-function-declaration"
export CFLAGS

make %{?_smp_mflags}
  - Well, inserting CFLAGS in this method actually does _not_ change
    CFLAGS (see the result: )

    The correct way is either
    - to export CFLAGS _before_ %configure (also please check what
      %configure does by $ rpm --eval %configure). This method is
      preferred than the latter. Recent autotool based Makefiles/configures
      usually accepts this method.
    - to use 
      make %{?_smp_mflags} CFLAGS=XXXXXXXXX
      This method should be used when the former method does not work.

* %files entry
%dir %{_datadir}/%{name}/
  - The entry %{_datadir}/%{name}/ contains the directory %{_datadir}/%{name}
    itself, so the first line is not needed (and should be removed)

  - %{_datadir}/icons/* contains all directories under %{_datadir}/icons,
    for example /usr/share/icons/hicolor (please check the files entry
    by $rpm -ql florence).
    /usr/share/icons/hicolor and so on are already owned by 
    hicolor-icon-theme and should not be owned by this package
    (the first line must be fixed)

Comment 15 Mamoru TASAKA 2008-07-27 15:18:25 UTC
One more thing
* desktop files
  - Category 'Application' is deprecated and should be removed
    (you can use --remove-category option)

Comment 17 Mamoru TASAKA 2008-07-27 18:41:28 UTC
Okay, then:

NOTE: Before being sponsored:

This package will be accepted with another few (or no) work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
(NOTE: please don't choose "Merge Review")

Review guidelines are described mainly on:

Comment 18 Simon 2008-07-29 16:27:23 UTC
I'm in a friendly contact with upstream. :-)


I will try to meet all your requirements.

Comment 19 Mamoru TASAKA 2008-07-30 07:45:23 UTC
Okay, good:

* This package itself is now good
* I noticed that you have another review request (bug 454166), which I
  am actually reviewing but I didn't notice that the submitter is you...

     This package (florence) is APPROVED by me

Please follow the procedure written on:
from "Install the Client Tools (Koji)".

I noticed that you have already requested packager membership so
now I am sponsoring you.

If you want to import this package into Fedora 8/9, you also have
to look at
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.


Comment 21 Mamoru TASAKA 2008-07-31 06:55:50 UTC
Okay. Please follow "Join" wiki.

Comment 22 Simon Wesp 2008-07-31 17:53:03 UTC
New Package CVS Request
Package Name: florence
Short Description: Extensible scalable on-screen virtual keyboard for GNOME
Owners: cassmodiah
Branches: F-8 F-9
Cvsextras Commits: yes

Comment 23 Kevin Fenzi 2008-08-01 15:57:01 UTC
cvs done.

Comment 24 Mamoru TASAKA 2008-08-03 12:56:02 UTC
Please close this bug as NEXTRELEASE when you rebuilt this package on
devel/F-9/F-8 and request on bodhi is done.

Comment 25 Mamoru TASAKA 2008-08-03 13:13:56 UTC
Closing as rebuild and request on bodhi is done.

Comment 26 Simon 2010-06-23 12:10:52 UTC
Package Change Request
Package Name: florence
New Branches: EL-6
Owners: cassmodiah

Comment 27 Jason Tibbitts 2010-06-26 07:27:48 UTC
CVS done (by

Comment 28 Christopher Meng 2014-05-19 01:35:16 UTC
Package Change Request
Package Name: florence
New Branches: epel7
Owners: cicku

Comment 29 Gwyn Ciesla 2014-05-19 12:00:40 UTC
Git done (by process-git-requests).

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