Bug 204700 - Review Request: njb-sharp - C# bindings to libnjb
Review Request: njb-sharp - C# bindings to libnjb
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul F. Johnson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-08-30 17:44 EDT by Linus Walleij
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-30 14:31:18 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
New spec file (3.03 KB, application/x-extension-spec)
2006-10-07 17:08 EDT, Paul F. Johnson
no flags Details
Patch (467 bytes, patch)
2006-10-07 17:09 EDT, Paul F. Johnson
no flags Details | Diff

  None (edit)
Description Linus Walleij 2006-08-30 17:44:16 EDT
Spec URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp-0.3.0-1.src.rpm
SRPM URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp.spec
Description: njb-sharp provides C# bindings to libnjb, which is
interesting for the music player Banshee for example.
Comment 1 Alex Lancaster 2006-09-09 08:14:59 EDT
Bug #188218 to update banshee to 0.10.12 would benefit from this package.  

In the meantime, is it possible to configure banshee without njb support?
Comment 2 Mamoru TASAKA 2006-10-07 11:39:29 EDT
I cannot rebuild this by mockbuild under FC-devel i386.

checking dynamic linker characteristics... cat: ld.so.conf.d/*.conf: No such
file or directory
GNU/Linux ld.so
checking how to hardcode library paths into programs... immediate
appending configuration tag "F77" to libtool
checking for i686-redhat-linux-gnu-pkg-config... no
checking for pkg-config... /usr/bin/pkg-config
checking pkg-config is at least version 0.9.0... yes
checking for NJB... yes
checking for mono... /usr/bin/mono
checking for mcs... /usr/bin/mcs
checking for monodocer... no
configure: error: You need to install monodoc
error: Bad exit status from /var/tmp/rpm-tmp.2121 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.2121 (%build)
Error building package from njb-sharp-0.3.0-1.fc6.src.rpm, See build log
Comment 3 Paul F. Johnson 2006-10-07 12:33:06 EDT
Okay, x things. 

First, are you intending to have this available for FC5 as well as FC6? If you
are then until mono in FC5 is updated, you'll need a hack otherwise it will be
broken for x86_64

Something like

%if "%{?fedora}" == "5"
%define monodir %{_prefix}/lib
%else
%define monodir %{_libdir}
%endif

You will then need patches for the makefile.am files which point statically to
/usr/lib and change them to @libdir@. These patches ONLY need to be applied if
you're not using them for FC5, so in %setup you have

%if "%{?fedora}" > "5"
%patch0 -p1
autoreconf
%endif

(obviously, this will mean you need to include the BRs for autoreconf - IIRC,
it's autoconf, automake and libtool)

2. Anything that is for a pkgconfig *must* be in it's own package - it doesn't
matter if it's a single line or not, it has to be in it's own -devel package.

3. You will need BR pkgconfig and monodoc (there may be some switch in the
configure script which can switch off monodoc). Me, I'd include it as the more
that goes in monodoc, the better.

4. Don't steel from SuSE - their spec files have been known to cause people's
brains to explode, implode and then make a sort of sqeeeeeeeee noise.

5. Not sure what you mean about these commented out lines

# These are for /usr/bin/monodoc, part of "mono-tools", not part of FC/FE 
# and not present in the current "mododoc" package.

If there is a problem with monodoc, please file a BZ report and I'll fix it.
Comment 4 Paul F. Johnson 2006-10-07 17:08:52 EDT
Created attachment 137981 [details]
New spec file

This spec file sorts out the problems already seen and includes the FC5
hackery. Packages built are pretty much clean with rpmlint. I've dropped the
makefile patch from the original version of the spec file as it breaks
everything under FC6/rawhide on x86_64
Comment 5 Paul F. Johnson 2006-10-07 17:09:46 EDT
Created attachment 137982 [details]
Patch

Patch file for the .pc.in file - it is not applied unless FC6 or above is being
used.
Comment 6 Paul F. Johnson 2006-10-07 17:14:53 EDT
Packages created with the above spec file are clean as well with mock (x86)
Comment 7 Linus Walleij 2006-10-08 08:06:08 EDT
OK, thanks A LOT Paul, I would never have sorted this out myself:

Spec URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp-0.3.0-3.src.rpm
SRPM URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp.spec

Only cosmetics on Pauls patch.
Comment 8 Paul F. Johnson 2006-10-08 08:43:55 EDT
Okay, the next tasks are these

1. Standardise the macros. I've used %{_bindir} and %{buildroot}. Your original
version used %_bindir et al. You need to have them all as %{} or %_

2. Have you tested to see if the parallel make fails or was this from a
different spec file?
Comment 9 Ralf Corsepius 2006-10-08 23:51:33 EDT
(In reply to comment #8)
> Okay, the next tasks are these
> 
> 1. Standardise the macros. I've used %{_bindir} and %{buildroot}. Your original
> version used %_bindir et al. You need to have them all as %{} or %_
Nope - %{_X} is the quoted version of %_X, they are identical.
Comment 10 Linus Walleij 2006-10-17 15:20:01 EDT
OK sorry for taking so long:
Spec URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp-0.3.0-4.src.rpm
SRPM URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp.spec

I fixed the escapes nevertheless, doesn't hurt.
Comment 11 Paul F. Johnson 2006-10-17 16:48:31 EDT
Okay, I'll build this in a minute.

Quick review notes.

1. The macro for %{monodir} is only temporary until FC5 mono is brought in line
with FC6

2. You need to be consistent with your macros - you have %{name} and %name as
well as %buildroot and %{buildroot}.

3. You need to remove the comment about parallel makes being broken - they're
not as it builds fine with them on.

4. Does this generate mono debug info as well as ELF debug info? This is just a
query as monodebugger can then handle it ;-)

5. The rm -f *a line needs to be *.a - *a means "any text that ends in a"
Comment 12 Paul F. Johnson 2006-10-17 16:50:37 EDT
parallel build is broken on x86
Comment 13 Linus Walleij 2006-10-21 17:31:28 EDT
Thanks Paul most of these are fixed not I think:
Spec URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp-0.3.0-5.src.rpm
SRPM URL: http://www.df.lth.se/~triad/krad/fc/njb-sharp.spec

I'm unsure regarding 4. mono debug infos, the ELF debug info is generated
by the build system, so I beleive it is a question for the builder machine
config right? (I never looked into it for ELF even.)
Comment 14 Paul F. Johnson 2006-10-21 17:37:55 EDT
Don't worry about #4 that much. What typically happens is that debuginfo for
mono tends to be built anyway.

rpmlint should be used before uploading as it saves a heck of a lot of time when
reviewing!

monotools is currently awaiting review (if that is of any help ;-p)
Comment 15 Linus Walleij 2006-10-21 17:47:03 EDT
Yeah OK, point taken (you must be refering to 0.3.0-1) it was a bit late at
night, my habit is nominally to rpmlint things first.
Comment 16 Paul F. Johnson 2006-11-02 17:05:33 EST
Anything happening on this package?
Comment 17 Linus Walleij 2006-11-03 06:56:49 EST
Hm, well I might have misunderstood something but as far as I know
I have fixed all issues. rpmlint is silent apart from a message about
the macros that can be ignored in this case I believe.

Parallell build is still broken but disabling parallell building
effectively fixes that for now.
Comment 18 Paul F. Johnson 2006-11-07 18:24:18 EST
rpmlint is clean, builds fine in mock (x86)

Review

Spec file clear and in US English
Consistent use of macros
FC-5 macro requires (permitted)
Upstream tarball md5 is the same as the packaged version
License good
pkgconfig included for devel package
Uses DESTDIR for the install
post and postun make sense
clean present
docs included in main package
owns correct files
Links with test code

Okay, I'm happy with this. Please close the bug and set as NEXT RELEASE

APPROVED

Please remember that when FC-5 comes up to date with FC-6's mono, the macro will
need to be replaced.
Comment 19 Paul F. Johnson 2006-11-25 16:48:57 EST
This package has been approved. Please close the bug and import (if you haven't
already)
Comment 20 Linus Walleij 2006-11-25 17:01:19 EST
The import was stalled for a few days due to the CVS outage, it was then
imported and now we are waiting for branches for FC-5 and FC-6:
http://fedoraproject.org/wiki/Extras/CVSSyncNeeded

The bug will be closed once it's built etc.

Thanks for keeping an eye on this Paul!
Comment 21 Linus Walleij 2006-11-30 14:31:18 EST
Package does not build under x86_64 on FC-5,
FC-6, devel unproblematic. Ithink we will simply
dump FC-5 support or wait until its mono gets in sync
with FC-6 & devel, so closing.

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