Bug 524707 - Review Request: chronojump - A measurement, management and statistics sport testing tool
Summary: Review Request: chronojump - A measurement, management and statistics sport t...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Michel Lind
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-21 20:15 UTC by Ismael Olea
Modified: 2009-10-06 23:43 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-10-06 23:43:52 UTC
Type: ---
Embargoed:
michel: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ismael Olea 2009-09-21 20:15:19 UTC
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 Lind 2009-09-21 23:42:35 UTC
> $ 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-22 00:32:42 UTC
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 Lind 2009-09-22 17:51:56 UTC
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 22:26:06 UTC
(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 09:37:10 UTC
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 Lind 2009-09-29 18:02:44 UTC
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 Lind 2009-10-04 21:05:38 UTC
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 21:41:55 UTC
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 21:42:47 UTC
(In reply to comment #8)

I take note of your suggestion

> APPROVED  

Thanks!

Comment 11 Kevin Fenzi 2009-10-06 17:44:26 UTC
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 20:32:28 UTC
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 20:33:01 UTC
(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 21:30:46 UTC
cvs done.


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