Bug 189315 - Review Request: ardour
Review Request: ardour
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Callum Lerwick
Fedora Package Reviews List
:
Depends On: 183912 189309 189313
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-18 22:14 EDT by Anthony Green
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-03 01:11:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Anthony Green 2006-04-18 22:14:14 EDT
Spec URL: http://people.redhat.com/green/FE/FC5/ardour.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/ardour-0.99.2-1.src.rpm
Description: 
Ardour is a multichannel hard disk recorder (HDR) and digital audio
workstation (DAW). It is capable of simultaneous recording 24 or more
channels of 32 bit audio at 48kHz. Ardour is intended to function as a
"professional" HDR system, replacing dedicated hardware solutions such
as the Mackie HDR, the Tascam 2424 and more traditional tape systems
like the Alesis ADAT series. It is also intended to provide the same
or better functionality as software systems such as ProTools,
Samplitude, Logic Audio, Nuendo and Cubase VST (we acknowledge these
and all other names as trademarks of their respective owners). It
supports MIDI Machine Control, and so can be controlled from any MMC
controller, such as the Mackie Digital 8 Bus mixer and many other
modern digital mixers.

This is based on an old planet ccrma spec file.

This package depends on jack-audio-connection-kit, liblrdf and raptor... all of which have been submitted to FE for review.
Comment 1 Tim Mayberry 2006-04-19 00:15:47 EDT
Are you aware that by default ardour's build system will build and statically
link to several libraries included in the release tarball which may already be
packaged in FE. The packaging guidelines say something about this as I recall.

There is a build flag to link dynamically with the system libraries, something
like SYSLIBS=1.

The URL in the spec file is also wrong, the ardour website was moved from
sourceforge to ardour.org

Thanks for submitting this, I hope I don't seem too critical of your effort.
Comment 2 Anthony Green 2006-04-19 03:53:15 EDT
(In reply to comment #1)
> Are you aware that by default ardour's build system will build and statically
> link to several libraries included in the release tarball which may already be
> packaged in FE.

Thanks for all the pointers.  I'll look into your comments.

> Thanks for submitting this, I hope I don't seem too critical of your effort.

Not at all.

AG
 

Comment 3 Andy Shevchenko 2006-04-19 05:19:26 EDT
Briefly check of spec file gives following remarks:
 - why do you not use %find_lang macro?
 - ardor needs own the /etc/ardour/ (and may be use %{_sysconfdir} instead of /
etc)
 - differnet style of build root definition (%buildroot vs. $RPM_BUILD_ROOT and 
 %{buildroot}%{_other_macro} vs. %{buildroot}/%{_other_macro})
 - buildrequires overhead (gcc-c++, flex and so on. See packaging guidelines)
 - more preferable to put full URL instead of name-version in Source: parameter
 - I don't know about noreplace feature in %config. Possible you need it.
Comment 4 Anthony Green 2006-04-19 13:27:22 EDT
(In reply to comment #3)
> Briefly check of spec file gives following remarks:
>  - why do you not use %find_lang macro?

How do I use this?  There are .mo files with different names (gtk_ardour.mo,
libardour.mo, etc).

>  - ardor needs own the /etc/ardour/ (and may be use %{_sysconfdir} instead of /
> etc)

Done.

>  - differnet style of build root definition (%buildroot vs. $RPM_BUILD_ROOT and 
>  %{buildroot}%{_other_macro} vs. %{buildroot}/%{_other_macro})

Fixed.

>  - buildrequires overhead (gcc-c++, flex and so on. See packaging guidelines)

Removed gcc-c++.  I believe flex is required.

>  - more preferable to put full URL instead of name-version in Source: parameter

Fixed.

>  - I don't know about noreplace feature in %config. Possible you need it.

Added.

Thanks!


Comment 5 Anthony Green 2006-04-21 07:50:55 EDT
(In reply to comment #3)
> Briefly check of spec file gives following remarks:

Updated spec file and SRPM here:

Spec URL: http://people.redhat.com/green/FE/FC5/ardour.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/ardour-0.99.2-2.src.rpm

Comment 6 Anthony Green 2006-04-21 08:09:04 EDT
(In reply to comment #1)
> Are you aware that by default ardour's build system will build and statically
> link to several libraries included in the release tarball which may already be
> packaged in FE. The packaging guidelines say something about this as I recall.

I looked into this a little.  The ardour sources are bundled with versions of
libsigc++, libgtkmm, and other libraries not currently packaged for FC/E.  The
packaging guidelines say that packages should only link against system
libraries, not private versions of their own libraries.  However, I think we
should make an exception in this case.  Here are my reasons:

- The versions of these libraries are very different from the versions packaged
in Fedora (by major version numbers). 
- The libraries have been modified by the ardour authors.
- The ardour authors only support the use of these statically linked modified
libraries.
- The ardour authors support these libraries within ardour as if it were their
own application code.  

Updating ardour to use the system libraries would be a major development effort
and would not have the support of the upstream project.

I think the fact that these are statically linked into the app (making it
impossible for another app to use my mistake) is also a mitigating factor here.

Please make an exception in this case.  Forcing ardour to use system versions of
these libraries would be counter-productive.
Comment 7 Paul Howarth 2006-04-21 08:16:05 EDT
Regarding Comment #5, I think this is a reasonable request.
The nmap package in Core similarly includes modified private versions of system
libraries.
Comment 8 Andy Shevchenko 2006-04-21 12:56:25 EDT
> How do I use this?  There are .mo files with different names (gtk_ardour.mo,
libardour.mo, etc).

This is example:
%install
...

%find_lang gtk_ardour
%find_lang libardour

for i in gtk_ lib; do
  cat ${i}ardour >> ardour.lang
done
...
%files -f ardour.lang
Comment 9 Michael Schwendt 2006-04-22 19:31:34 EDT
Simply the following will do, too (but I admit it's ugly):

%find_lang .\*ardour.\*

%files -f .\*ardour.\*.lang


For those interested in the details, the argument of %find_lang
is a regular expression _and_ filename prefix. The argument of
%files -f is a filename.
Comment 10 Anthony Green 2006-04-22 23:00:54 EDT
(In reply to comment #8)
> This is example:

Thanks.  Here are updated bits using %find_lang...

Spec URL: http://people.redhat.com/green/FE/FC5/ardour.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/ardour-0.99.2-3.src.rpm

I believe this SRPM addresses every point raised so far.  Thanks!

Comment 11 Anthony Green 2006-05-13 12:20:19 EDT
Updated to 0.99.3 sources...

Spec URL: http://people.redhat.com/green/FE/FC5/ardour.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/ardour-0.99.3-1.src.rpm

Comment 12 Anthony Green 2006-05-13 17:08:08 EDT
Read ladspa plugins from lib64 dirs on x86-64 systems....

Spec URL: http://people.redhat.com/green/FE/FC5/ardour.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/ardour-0.99.3-2.src.rpm
Comment 13 Callum Lerwick 2006-07-24 02:02:43 EDT
Alright, lets see if we can get this in before FC4 goes into maintenance mode.

rpmlint on the src.rpm says:

W: ardour rpm-buildroot-usage %build rm -f
$RPM_BUILD_ROOT%{_libdir}/ardour/libardour*
$RPM_BUILD_ROOT should not be touched during %build or %prep stage, as it
will break short circuiting.

I'm really not sure what this command even accomplishes. Old cruft? Delete it.

E: ardour tag-not-utf8 %changelog
E: ardour non-utf8-spec-file ardour.spec

The changelog is a mess. No blank lines between the older entries, etc. I'm
thinking maybe you should just cut off all the old CCRMA entries, which would
make the encoding errors moot. They seem to be mostly irrelevant to the current
state of the package except for the explanation of the NOARCH and ARCH options
which should be retained as a comment up in the %build section.

W: ardour macro-in-%changelog find_lang
W: ardour macro-in-%changelog _sysconfdir

Remember you need to escape macros in the changelog, i.e. %%find_lang. Or just
drop the % entirely.

I see an %{__install} macro in there, which is inconsistent. There's also some
%{name} macros in there I'd personally lose.

I wouldn't bother packaging the README, it contains basically nothing of use.

The following buildreqs do not seem to be necessary: bison, byacc, flex,
linuxdoc-tools, curl-devel

The %files section could use some cleanup. Put trailing slashes on the
directories, and I'd recommend getting rid of the globbing. And get rid of the
%{name} macro.

During the build, RPM warns of files listed twice. Add %dir to
%{_sysconfdir}/ardour to fix it.
Comment 14 Anthony Green 2006-07-31 01:33:55 EDT
(In reply to comment #13)
> Alright, lets see if we can get this in before FC4 goes into maintenance mode.

I fixed everything you pointed out.  Updated bits here:

Spec URL: http://people.redhat.com/green/FE/FC5/ardour.spec
SRPM URL: http://people.redhat.com/green/FE/FC5/ardour-0.99.3-3.src.rpm

Thanks!

Comment 15 Callum Lerwick 2006-08-05 19:34:29 EDT
MUST items:

- rpmlint: Ok
- Package name: Ok
- Spec name: Ok
- Meets packaging guidelines: Ok
- License: Ok
- Spec in American English: Ok
- Spec legible: Ok
- Sources match upstream: Ok
- Builds: Ok
- BuildRequires: Ok
- Locales: Ok
- ldconfig: Ok
- Relocation: Ok
- Directory ownership: Ok
- %files: Ok
- %clean: Ok
- Macros: Ok
- Code vs. Content: Ok
- Documentation: Ok
- devel package: Ok
- .desktop file: Ok

SHOULD:

- Includes license text: Ok
- Mock build: Ok
- Builds on all archs: Built on i386, x86_64
- Package functional: Tested on i386, x86_64
- Scriptlets: Ok
- Subpackages: Ok

NEEDSWORK:

Just noticed the desktop entry is embedded in the spec. Putting it in a file and
sourcing it would be preferable.

Other than that, looks good.
Comment 16 Callum Lerwick 2006-09-02 15:29:51 EDT
As discussed in IRC, move the desktop file out of the spec before import and
this is APPROVED.
Comment 17 Anthony Green 2006-09-03 01:11:23 EDT
(In reply to comment #16)
> As discussed in IRC, move the desktop file out of the spec before import and
> this is APPROVED.

Thanks.  I've submitted this to the build system.

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