Bug 227256 - Review Request: moto4lin - Filemanager and seem editor for Motorola P2k phones
Review Request: moto4lin - Filemanager and seem editor for Motorola P2k phones
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
: Reopened
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2007-02-03 20:28 EST by jafo-redhat
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-04 17:03:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description jafo-redhat 2007-02-03 20:28:48 EST
Spec URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin.spec
SRPM URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin-0.3-1.src.rpm
Description: 
This software is Filemanager and seem editor for Motorola P2k phones
(like C380/C650).
Comment 1 manuel wolfshant 2007-02-04 07:03:34 EST
MUSTFIX:
Missing SMP flags: if there is a known problem, a comment should be added in the
spec file (wiki: PackagingGuidelines#parallelmake)
You should not use the Packager tag (wiki: PackagingGuidelines#tags)
Does not build in mock (missing qmake), you should add a Buildrequire for qt-devel.

Minor:
BuildRoot does not follow the guidelines (should be
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) (wiki:
PackagingGuidelines#BuildRoot)
Comment 2 jafo-redhat 2007-02-04 07:27:31 EST
Thanks for the review.  New version addressing these is at:

Spec URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin.spec
SRPM URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin-0.3-2.src.rpm
Comment 3 manuel wolfshant 2007-02-04 08:06:18 EST
No need to mention in the Changelog who suggested what, especially when dealing
with errors :)

I'll review that. Note that I cannot actually test the program because I do not
have a compatible phone.
Comment 4 manuel wolfshant 2007-02-04 09:05:17 EST
GOOD
- package meets naming guidelines
- license ( GPL ) OK, matches source
- spec file legible, in am. english
- source matches upstream, sha1sum 
12dff499f7a29a36e7b7a67d3260d470280485dc  moto4lin-0.3.tar.bz
- package compiles on FC6/x86_64
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for separated -doc
- nothing in %doc affects runtime
- no .la, static, .pc files

MUSTFIX
- the source rpm should retain the date of the upstream source
- the binary rpm should include as %doc the license file (it is included in the
tar.bz2 as GPL-2)
- %prep does not need to include cleaning the buildroot
- the program is a GUI, so acording to
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop a moto4lin.desktop
file should be included in the rpm. Otherwise a comment with your explanation of
not providing one should be included in the spec file

Suggestion
- It is unusual for a spec to include the comments about the macros. If you
definitely need them, feel free to let them in, but otherwise they should be
deleted.

NEEDINFO
- as far as I can tell, the program does start in FC6. Without a compatible
phone I cannot say if it actually works. The comment on the main page of the
moto4lin wiki states that the 0.3 version does not work on x86_64 and recommends
either patching it or using the more recent svn version. Could you please
elaborate on this aspect?
Comment 5 manuel wolfshant 2007-02-04 09:08:57 EST
"- the source rpm should retain the date of the upstream source" ==> the date of
upstream tar.bz2 should be preserved
Comment 6 jafo-redhat 2007-02-04 22:31:01 EST
All these issues are resolved.  The comments were in there from another source,
I don't care about them so I removed them.

The program runs on my FC6 system, but I haven't copied anything on or off my
phone.  Scott, who's package this is based off, says he used it to copy photos
off his phone.
Comment 7 manuel wolfshant 2007-02-05 01:57:06 EST
The package looks almost fine now, except for
- in Fedora there is no lib64usb-devel. Independent of architecture the package
is called libusb-devel.
- the Categories section in the .desktop does not look right. I for one would
not look for this app under "Graphics" but under "Communications" or something
similar. Maybe that, for inspiration sake, you could take a pick at some other
similar application ?
- patches are to be included as %Patch, not as %Source. I suggest

-Source3:        %{name}-0.3-crossplatform.patch
+Patch:          %{name}-0.3-crossplatform.patch
-patch -p1 -s <%{SOURCE3}
+%Patch -p1 -b crossplatform

Please address the above and I will be glad to do a final review.
Comment 9 manuel wolfshant 2007-02-05 21:40:40 EST
Just checked the new version. These seem to be the problems left:
- Since release 3, you do not use a consistent convention for the Buildroot.
Please select either $RPM_BUILD_ROOT or %{buildroot} and use it everywhere
(http://fedoraproject.org/wiki/Packaging/Guidelines#UsingBuildRootOptFlags)
- Packaging/Guidelines/Compiler flags is not respected. The build process is
based on qmake.conf default settings, not on $RPM_OPT_FLAGS
- the .desktop file does not use a standard category. Please try to find an
appropriate one from http://standards.freedesktop.org/menu-spec/latest/apa.html
- I am not sure that Applications/File is the proper RPM Group (I feel like
Applications/Communications being more appropriate since you communicate with
the mobile..) but I will not object if you retain it. Maybe someone more
experienced could give us a hint here ?
Comment 10 jafo-redhat 2007-02-11 16:57:36 EST
I've uploaded new spec and SRPM files, and have researched qmake and determined
that the optimizer flags isn't something I can fix.

SRPM URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin-0.3-5.src.rpm
Spec URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin.spec
RPM URL: ftp://ftp.tummy.com/pub/tummy/RPMS/FC6/moto4lin-0.3-5.i386.rpm
Comment 11 manuel wolfshant 2007-02-13 03:56:54 EST
I have reviewed this release (moto4lin-0.3-5) and all problems reported before
seem fixed, except for one: the compile step does not take into account
$RPM_OPT_FLAGS: qmake ignores the parameters included in the the spec file and
uses only those included in the project bundled in the source, which in turn
uses the default values from /usr/lib/qt-3.3/mkspecs/linux-g++/qmake.conf.
Unfortunately this contradicts the packaging guidelines (wiki: Compiler flags).
Neither the reporter not I have enough experience to solve this issues,
therefore I kindly request for counseling from people more experienced.
Comment 12 Mamoru TASAKA 2007-02-13 04:09:39 EST
I think giving up this package is too early. Back to assigned.
I will see if I can help for this.
Comment 13 Mamoru TASAKA 2007-02-13 04:16:45 EST
make CXX="g++ $RPM_OPT_FLAGS" %{_smp_mflags} all
seems okay.
Comment 14 manuel wolfshant 2007-02-13 04:29:34 EST
Thank you, Mamoru, that was it.

Jafo-redhat: please modify the spec to include Mamoru's suggestion and I will
approve the package.
Comment 15 jafo-redhat 2007-02-13 16:28:33 EST
Thanks Mamoru, I've built new packages with your build flags.

SRPM URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin-0.3-6.src.rpm
Spec URL: ftp://ftp.tummy.com/pub/tummy/RPMS/SRPMS/moto4lin.spec
Comment 16 manuel wolfshant 2007-02-13 19:38:21 EST
All problems seem fixed. Package APPROVED.
Comment 17 Pikachu_2014 2007-02-19 18:54:36 EST
I would like to make a little suggestion : I have tested the RPM on my mobile
phone (a Motorola Razr V3) and with the version 0.3 of moto4lin, it doesn't work.
But with the SVN version --- moreover it is confirmed by the Moto4lin
developpers -- the SVN version supports more phones models, like mine.
Comment 18 Mamoru TASAKA 2007-03-03 06:03:29 EST
What is the status of this bug?
Comment 19 jafo-redhat 2007-03-04 17:03:29 EST
Sorry, forgot to get it closed.

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