Bug 435064 - Review Request: midisport-firmware - firmware files for M-Audio/Midiman USB MIDI and Audio devices
Review Request: midisport-firmware - firmware files for M-Audio/Midiman USB M...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-26 23:14 EST by Dave Jones
Modified: 2015-01-04 17:30 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-09 06:31:40 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Dave Jones 2008-02-26 23:14:48 EST
Spec URL: http://www.codemonkey.org.uk/junk/midisport-firmware.spec
SRPM URL: http://www.codemonkey.org.uk/junk/midisport-firmware-1.2-1.src.rpm
Description: Firmware for M-Audio/Midiman USB MIDI and Audio devices

I based this specfile on the ivtv-firmware spec.

TODO: add udev rules to autoload firmware.
Comment 1 Dave Jones 2008-02-28 11:16:53 EST
Updated to add udev rules.
Comment 2 Lubomir Kundrak 2008-03-03 14:13:15 EST
  3 Release:        1

It is fine that this doesn't contain dist tag as this package doesn't ship any
executable and is release independent. (Just a note to me and other reviewers
who may eventually look at this.)

  6 License:        GPL

This is no longer valid, it needs the version specification, e.g. GPL+, GPLv2 or
GPLv3+

According to LICENSE file this is GPLv2+

 18 %setup -q -c

No need for -c, tarball actually contains midisport-firmware-1.2 directory

 26 install -pm 0644 *.{fw,mpg} $RPM_BUILD_ROOT/lib/firmware

This is not correct, this package has not *.fw or *.mpg files. Did you mean
*.ihx? It doesn't build due to this. I'll continue the review once it builds.

 28 # Install the license with the firmware
 29 mv LICENSE license-midisport.txt

What was the reason for installing license in /lib/firmware? It should be
included in %doc instead. Forthermore, please add README to %doc (and probably
also ChangeLog).

 31 install -pm 0644 81-midisport.rules $RPM_BUILD_ROOT/etc/udev/rules.d/
...
 46 /etc/udev/rules.d/81-midisport.rules

Please replace occurences of /etc with %{_sysconfdir}

 49 * Thu Feb 28 2008 Dave Jones <davej@redhat.com>

Please include - version-release (sans disttag, " - 1.2-1" in this case) at the
end of changelog header
Comment 3 Lubomir Kundrak 2008-03-03 14:20:03 EST
Something from rpmlint:

midisport-firmware.noarch: E: description-line-too-long This package contains
the firmware for M-Audio/Midiman USB MIDI and Audio devices
Your description lines must not exceed 79 characters. If a line is exceeding
this number, cut it to fit in two lines.

midisport-firmware.noarch: W: non-conffile-in-etc
/etc/udev/rules.d/81-midisport.rules
A non-executable file in your package is being installed in /etc, but is not
a configuration file. All non-executable files in /etc should be configuration
files. Mark the file as %config in the spec file.

Please mark the udev rules file as %config. rpmlint will probably complain that
it's not %config(noreplace). That's fine, ignore it.

And some more issues:

 32 install -pm 0644 81-midisport.rules $RPM_BUILD_ROOT/etc/udev/rules.d/

Replace 81-midisport.rules with %{SOURCE1}
Create the directory $RPM_BUILD_ROOT%{_sysconfdir}/udev/rules.d/ prior to
copying to it.

That should be all.

Comment 4 Lubomir Kundrak 2008-03-03 14:24:29 EST
One more thing; you rules seem to be incorrect.

SUBSYSTEM=="usb", ENV{PRODUCT}=="763/1010/*", RUN+="/sbin/fxload -D
$env{DEVNAME} -s /etc/firmware/MidiSportLoader.ihx -I
/etc/firmware/MidiSport1x1.ihx"
SUBSYSTEM=="usb", ENV{PRODUCT}=="763/1020/*", RUN+="/sbin/fxload -D
$env{DEVNAME} -s /etc/firmware/MidiSportLoader.ihx -I
/etc/firmware/MidiSport4x4.ihx"

You install the files into /lib/firmware, not /etc/firmware
Comment 5 Lubomir Kundrak 2008-03-03 14:28:04 EST
And yet one more cosmetical one:

Change the Source0 tag not to point to a specific mirror:
http://download.sourceforge.net/usb-midi-fw/midisport-firmware-%{version}.tar.gz
Comment 6 Dave Jones 2008-03-03 15:18:37 EST
Both files updated with comments from above. Thanks for the review.
Comment 7 Lubomir Kundrak 2008-03-03 15:46:52 EST
install -pm 0644 *.{*.ihx} $RPM_BUILD_ROOT/lib/firmware
                 ^^^^^^^^^
You didn't mean this, did you?

%doc license-midisport.txt README Changelog
license file is called LICENSE and not license-midisport.txt

-* Thu Feb 28 2008 Dave Jones <davej@redhat.com> - 1.2
+* Thu Feb 28 2008 Dave Jones <davej@redhat.com> - 1.2-1
Comment 8 Dave Jones 2008-03-03 16:41:32 EST
fixed.
Comment 9 Lubomir Kundrak 2008-03-03 16:59:31 EST
Thanks for the package, Dave!

APPROVED
Comment 10 Dave Jones 2008-03-03 17:54:09 EST
New Package CVS Request
=======================
Package Name: midisport-firmware
Short Description: firmware for M-Audio/Midiman USB MIDI & Audio device
Owners: davej
Branches: 
InitialCC: 
Cvsextras Commits: yes
Comment 11 Kevin Fenzi 2008-03-03 22:02:15 EST
cvs done.

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