Bug 524707 - Review Request: chronojump - A measurement, management and statistics sport testing tool
Review Request: chronojump - A measurement, management and statistics sport t...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Michel Alexandre Salim
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-21 16:15 EDT by Ismael Olea
Modified: 2009-10-06 19:43 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-10-06 19:43:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
michel: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ismael Olea 2009-09-21 16:15:19 EDT
Spec URL: http://olea.org/tmp/chronojump.spec
SRPM URL: http://olea.org/paquetes-rpm/fedora-11/chronojump-0.8.10-1.fc11.src.rpm

Description:

ChronoJump is an open hardware, free software, multiplatform complete system
for measurement, management and statistics of sport short-time tests.

Chronojump uses a contact platform and/or photocells,
and also a chronometer printed circuit designed ad-hoc in
order to obtain precise and trustworthy measurements.

Chronojump is used by trainers, teachers and students.

CJ is an awarded project supported by the active research work of Xavier de Blas


$ rpmlint -iv chronojump-0.8.10-1.fc11.src.rpm
chronojump.src: I: checking
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -iv chronojump-0.8.10-1.fc11.i586.rpm 
chronojump.i586: I: checking
chronojump.i586: W: devel-file-in-non-devel-package /usr/lib/chronojump/libchronopic.so
A development file (usually source code) is located in a non-devel package. If
you want to include source code in your package, be sure to create a
development package.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.


$ rpmlint -iv chronojump-doc-0.8.10-1.fc11.i586.rpm 
chronojump-doc.i586: I: checking
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

I know it's expected to have clean rpmlint but it's weird for me to create an devel package for libchronopic.so only, specially considering is not expected to be used for developing.
Comment 1 Michel Alexandre Salim 2009-09-21 19:42:35 EDT
> $ rpmlint -iv chronojump-0.8.10-1.fc11.i586.rpm 
> chronojump.i586: I: checking
> chronojump.i586: W: devel-file-in-non-devel-package
> /usr/lib/chronojump/libchronopic.so
> A development file (usually source code) is located in a non-devel package. If
> you want to include source code in your package, be sure to create a
> development package.

If this is not meant for other software to use, then it can be ignored, yes.

> I know it's expected to have clean rpmlint but it's weird for me to create an
> devel package for libchronopic.so only, specially considering is not expected
> to be used for developing.  
Most Mono packages don't have clean rpmlint outputs either, just because rpmlint does not consider .dll and .exe to be libraries and executables :)
Comment 2 Jason Tibbitts 2009-09-21 20:32:42 EDT
Please try to follow the usual format for review tickets; we need to be able to search the ticket summary for the package name.
Comment 3 Michel Alexandre Salim 2009-09-22 13:51:56 EDT
Looks good so far! There are several minor things to fix, and one major thing -- the launcher script is broken on 64-bit archs -- the latter is a common
problem with our Mono apps, so I guess it's a good thing it's caught during
review anyway.

Fix them and I can approve this.

Koji F-12 build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1698459


ReviewTemplate

MUST

OK rpmlint

$ rpmlint ~/rpmbuild/SRPMS/chronojump-0.8.10-1.fc11.src.rpm ./chronojump*
error checking signature of /home/michel/rpmbuild/SRPMS/chronojump-0.8.10-1.fc11.src.rpm
chronojump.x86_64: W: devel-file-in-non-devel-package /usr/lib64/chronojump/libchronopic.so
4 packages and 0 specfiles checked; 0 errors, 1 warnings.


Internal use only, warning can be ignored


OK package name
OK spec file name
OK package guideline-compliant
OK license complies with guidelines
OK license field accurate
OK license file not deleted
OK spec in US English
OK spec legible
OK source matches upstream
OK builds under >= 1 archs, others excluded: Koji
?  build dependencies complete
   over-complete: you can drop BR on mono-core since you already BR: mono-devel
OK locales handled using %find_lang, no %{_datadir}/locale
FIX library -> ldconfig
   since your package does not install any library in the normal ld.so path
   and does not extend the path (see /etc/ld.so.conf*), running ldconfig is
   unnecessary (this also gives you the exception from the .so-in-devel rule)

FIX own all directories
  must depend on hicolor-icon-theme for the installed icon
OK no dupes in %files
OK permission
OK %clean RPM_BUILD_ROOT
OK macros used consistently
  not really about macros, but about scriplets. See the guideline about how to
  

OK Package contains code
?  large docs => -doc
   -doc should be in group "Documentation"

   Also, you can make -doc noarch, starting from RPM 4.6 (i.e. F-10 and above)
   and since Mono is not available on RHEL, you can just go ahead and declare
   the subpackage to be noarch without testing for the Fedora/RHEL version first

e.g.
   %package doc
   ...
   BuildArch: noarch

OK doc not runtime dependent
NA if contains *.pc, req pkgconfig
NA if libfiles are suffixed, the non-suffixed goes to devel
OK desktop file uses desktop-file-install
OK clean buildroot before install
OK filenames UTF-8

SHOULD
OK package build in mock on all architectures
FIX package functioned as described
  the launcher script is hardcoded to look for the .exe under /usr/lib instead
  of %{_libdir}:
$ chronojump
Cannot open assembly '/usr/lib/chronojump/Chronojump.exe': No such file or directory.

Unfortunately, a common problem in Mono apps, since Novell's openSUSE is also
multilib-capable (like Fedora) and thus they intentionally hard-code /usr/lib
because they consider Mono to be noarch, and we don't make that assumption :(

FIX scriplets are sane
  icon cache not updated; see
  http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
OK other subpackages should require versioned base
OK require package not files
Comment 4 Ismael Olea 2009-09-22 18:26:06 EDT
(In reply to comment #3)

> Fix them and I can approve this.

Everything has been fixed. I've added the scriplets for 
update-desktop-database

I don't have an x64 system so I can't test the launcher script but I think it's working right.


Spec URL: http://olea.org/tmp/chronojump.spec
SRPM URL: http://olea.org/paquetes-rpm/fedora-11/chronojump-0.8.10-2.olea.src.rpm
Comment 5 Ismael Olea 2009-09-29 05:37:10 EDT
Updated to 0.8.11:

Spec URL: http://olea.org/tmp/chronojump.spec
SRPM URL: http://olea.org/paquetes-rpm/fedora-11/chronojump-0.8.11-1.fc11.src.rpm

If I understood right today would be the last day for get into F12. Any news about the revision?
Comment 6 Michel Alexandre Salim 2009-09-29 14:02:44 EDT
Sorry about the delay. And no, you can still get it into F-12, you just have to get your package manually tagged. I'll help you through that if you want.

There is one fix you need to make: in src/util.cs, GetManualDir() returns GetPrefixDir() + "share/doc/chronojump"; the latter should really be

"share/doc/chronojump-doc-%{version}"

you'd probably need to fix it with sed, in the %prep section, since the version number will change each time.
Comment 8 Michel Alexandre Salim 2009-10-04 17:05:38 EDT
Just one thing -- rather than creating a tempfile, sed-ing into it and then renaming the temp, why not use sed -i (in-place) instead?

Also, sed can use different separator tokens, so when the string you want to replace is full of slashes, use something else -- say pipe -- instead

sed -i 's|share/doc/chronojump|share/doc/chronojump-%{version}|g' src/utils.cs

Otherwise, you might want to use $(mktemp) rather than `mktemp` -- backquote is a bash-ism (I just noticed it being frowned on in the Python merge review).

The program's output is *very* noisy, but I guess that's more of an upstream problem.

But you can make these changes when you commit the files to CVS -- everything else looks good.

APPROVED
Comment 9 Ismael Olea 2009-10-04 17:41:55 EDT
New Package CVS Request
=======================
Package Name: Chronojump
Short Description: A measurement, management and statistics sport testing tool
Owners: olea
Branches: F-10 F-11 F-12 
InitialCC: salimma
Comment 10 Ismael Olea 2009-10-04 17:42:47 EDT
(In reply to comment #8)

I take note of your suggestion

> APPROVED  

Thanks!
Comment 11 Kevin Fenzi 2009-10-06 13:44:26 EDT
You have a uppercase "Chronojump" in your request, but the package seems to be "chronojump" 
can you confirm the correct case and reset the fedora-cvs flag to ? when you are ready?
Comment 12 Ismael Olea 2009-10-06 16:32:28 EDT
New Package CVS Request
=======================
Package Name: chronojump
Short Description: A measurement, management and statistics sport testing tool
Owners: olea
Branches: F-10 F-11 F-12 
InitialCC: salimma
Comment 13 Ismael Olea 2009-10-06 16:33:01 EDT
(In reply to comment #11)
> You have a uppercase "Chronojump" in your request, but the package seems to be
> "chronojump" 
> can you confirm the correct case and reset the fedora-cvs flag to ? when you
> are ready?  

Sorry!
Comment 14 Kevin Fenzi 2009-10-06 17:30:46 EDT
cvs done.

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