Bug 454208 - Review Request: florence - Florence is an extensible scalable on-screen virtual keyboard for GNOME
Review Request: florence - Florence is an extensible scalable on-screen virt...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-06 13:49 EDT by Simon
Modified: 2014-05-19 08:00 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-03 09:13:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
florence-0.2.2-warnings.patch (3.94 KB, patch)
2008-07-26 16:25 EDT, Robert Scheck
no flags Details | Diff

  None (edit)
Description Simon 2008-07-06 13:49:13 EDT
Spec URL: http://packages.cassmodiah.de/fedora/florence/florence.spec
SRPM URL: http://packages.cassmodiah.de/fedora/florence/florence-0.2.2-1.fc9.src.rpm
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: https://bugzilla.redhat.com/show_bug.cgi?id=454166
Comment 1 Simon 2008-07-06 15:48:23 EDT
I made a big mistake

please CANCEL review request

I have to work on it
Comment 2 manuel wolfshant 2008-07-06 16:21:59 EDT
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 10:33:00 EDT
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".

* CFLAGS
  - The following lines:
---------------------------------------------------------------
CFLAGS="${CFLAGS:-$RPM_OPT_FLAGS}"
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 \
        %{buildroot}/%{_datadir}/applications/%{name}.desktop
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
    http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf
    %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
-----------------------------------------------------------------------
%files
%defattr(-,root,root,-)
%{_datadir}/%{name}/
-----------------------------------------------------------------------
    This contains the directory %{_datadir}/%{name} itself and all files/
    directories/etc under %{_datadir}/%{name}, while
-----------------------------------------------------------------------
%files
%dir %{_datadir}/%{name}/
------------------------------------------------------------------------
    contains the directory %{_datadir}/%{name} only.

* Documents
  - Please add the following files to %doc:
------------------------------------------------------------------------
COPYING-DOCS
NEWS
------------------------------------------------------------------------
Comment 5 Mamoru TASAKA 2008-07-18 10:40:41 EDT
BTW the description "Florence is" is redundant for Summary.
Comment 6 Mamoru TASAKA 2008-07-18 11:33:10 EDT
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 14:50:08 EDT
Well,

* 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
'layoutreader_readkeyboard'
-------------------------------------------------------------------
    Would you try to remove "implicit declaration of function' warning?
    c.f.
    https://www.redhat.com/archives/fedora-devel-list/2008-March/msg02036.html
Comment 9 Robert Scheck 2008-07-26 14:02:46 EDT
(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.
>     https://www.redhat.com/archives/fedora-devel-list/2008-March/msg02036.html

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 14:19:53 EDT
(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.
> >     https://www.redhat.com/archives/fedora-devel-list/2008-March/msg02036.html
> 
> 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
upstream,
Fedora's maintainer won't fix it' can _never_ be Fedora's policy.
Comment 11 Mamoru TASAKA 2008-07-26 14:22:57 EDT
ref:
https://bugzilla.redhat.com/show_bug.cgi?id=433182
Comment 12 Robert Scheck 2008-07-26 16:25:29 EDT
Created attachment 312715 [details]
florence-0.2.2-warnings.patch

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 11:13:34 EDT
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
-------------------------------------------------------------
%configure

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:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=741776 )

    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
  A.
-------------------------------------------------------------
%dir %{_datadir}/%{name}/
%{_datadir}/%{name}/
-------------------------------------------------------------
  - The entry %{_datadir}/%{name}/ contains the directory %{_datadir}/%{name}
    itself, so the first line is not needed (and should be removed)

  B.
-------------------------------------------------------------
%{_datadir}/icons/*
%{_datadir}/icons/hicolor/*/apps/%{name}.svg
-------------------------------------------------------------
  - %{_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 11:18:25 EDT
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 14:41:28 EDT
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 :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

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:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------
Comment 18 Simon 2008-07-29 12:27:23 EDT
I'm in a friendly contact with upstream. :-)

SPEC:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.3-1/florence.spec
SRPM:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.3-1/florence-0.2.3-1.fc9.src.rpm

I will try to meet all your requirements.
Comment 19 Mamoru TASAKA 2008-07-30 03:45:23 EDT
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:
http://fedoraproject.org/wiki/PackageMaintainers/Join.
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
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.
Comment 21 Mamoru TASAKA 2008-07-31 02:55:50 EDT
Okay. Please follow "Join" wiki.
Comment 22 Simon Wesp 2008-07-31 13:53:03 EDT
New Package CVS Request
=======================
Package Name: florence
Short Description: Extensible scalable on-screen virtual keyboard for GNOME
Owners: cassmodiah
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes
Comment 23 Kevin Fenzi 2008-08-01 11:57:01 EDT
cvs done.
Comment 24 Mamoru TASAKA 2008-08-03 08:56:02 EDT
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 09:13:56 EDT
Closing as rebuild and request on bodhi is done.
Comment 26 Simon 2010-06-23 08:10:52 EDT
Package Change Request
======================
Package Name: florence
New Branches: EL-6
Owners: cassmodiah
Comment 27 Jason Tibbitts 2010-06-26 03:27:48 EDT
CVS done (by process-cvs-requests.py).
Comment 28 Christopher Meng 2014-05-18 21:35:16 EDT
Package Change Request
======================
Package Name: florence
New Branches: epel7
Owners: cicku
Comment 29 Gwyn Ciesla 2014-05-19 08:00:40 EDT
Git done (by process-git-requests).

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