Bug 437691 - Review Request: monitor-edid - Tool for probing and parsing EDID
Review Request: monitor-edid - Tool for probing and parsing EDID
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2008-03-16 10:41 EDT by Remi Collet
Modified: 2008-04-07 01:13 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2008-04-07 01:13:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ville.skytta: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)
Use system lrmi (767 bytes, patch)
2008-04-05 16:58 EDT, Ville Skyttä
no flags Details | Diff

  None (edit)
Description Remi Collet 2008-03-16 10:41:18 EDT
Spec URL: http://remi.fedorapeople.org/monitor-edid.spec
SRPM URL: http://remi.fedorapeople.org/monitor-edid-1.16-1.fc8.src.rpm
Koji scratch build : http://koji.fedoraproject.org/koji/taskinfo?taskID=518430

Tool for probing and parsing EDID : http://en.wikipedia.org/wiki/EDID

This package will try to read the monitor details directly from the monitor.

This is an optional package for ocsinventory-agent which allow the retrieval of monitor informations, specially the Serial Number.
Comment 1 Ville Skyttä 2008-03-16 15:24:21 EDT
License: GPLv2 is incorrect.  The tarball includes COPYING which is the LGPLv2
text, but none of the source files have any GPL or LGPL references, instead they
have various BSD/MIT like notices.

Providing a checkout script would be more convenient than including comments in
the specfile.  Could also use svn export instead of checkout (does not checkout
.svn directories), and bzip2 or lzma the tarball instead of gzipping to save space.

Looks also like there's a private copy of lrmi in the tarball, is there a reason
why the Fedora packaged one is not used instead?

Including the word "monitor" in the Summary would be good.

Will continue the review after the license issues have been sorted out.
Comment 2 Remi Collet 2008-03-16 16:11:08 EDT
Thanks for the comments 

Spec URL: http://remi.fedorapeople.org/monitor-edid.spec
SRPM URL: http://remi.fedorapeople.org/monitor-edid-1.16-2.fc8.src.rpm

- Fix licenses : GPLv2 + MIT + DSB
- add LICENSE.x86emu in doc
- add monitor-edid-makesource.sh (I hope I well understand your comment)

lrmi is only an i386 package.
The version provided is build with x86emu to run under x86_64 (i don't think we
can use a i386 in mock/koji).
Comment 3 Bill Nottingham 2008-03-17 11:32:29 EDT
Augh, *another* LRMI/x86emu port? So this, radeontool, etc., etc. have to all
include separate versions?

Note that you really want to use x86emu everywhere.
Comment 4 Ville Skyttä 2008-03-17 16:16:52 EDT
(In reply to comment #2)
> - Fix licenses : GPLv2 + MIT + DSB

I still don't see anything GPLv2 in the package.  AFAICT GPLv2 should be
replaced by LGPLv2+, see COPYING included in the tarball and
Comment 5 Remi Collet 2008-03-17 16:41:04 EDT
@Bill a solution could be to ask kevin to provide and lrmi version for x86_64 ?

@Ville Argh, you're right.

Spec URL: http://remi.fedorapeople.org/monitor-edid.spec
SRPM URL: http://remi.fedorapeople.org/monitor-edid-1.16-3.fc8.src.rpm

Comment 6 Remi Collet 2008-04-05 14:37:44 EDT
@Ville Skyttä is there is still a problem with this package ?
Comment 7 Ville Skyttä 2008-04-05 16:58:51 EDT
Created attachment 301389 [details]
Use system lrmi

Using the private lrmi is not nice, something like the attached patch could do
the trick.  I have no way to check if the result actually works on i386, but it
does build.  If you apply this, also add %ifarch %{ix86} "BuildRequires:
lrmi-devel" to the specfile and "rm -f lrmi.h lrmi.c" in %prep to make sure the
bundled lrmi is not used.

MIT should probably be dropped from licenses even if system lrmi is not used;
lrmi gets built into an LGPLv2+ executable which I think makes the combined
work LGPLv2+ and we only list licenses for binaries.

%description could be improved, suggestion:
Monitor-edid is a tool for probing and parsing Extended display identification
data (EDID) from monitors.  For more information about EDID, see
Comment 8 Remi Collet 2008-04-06 03:42:50 EDT
Thanks for the patch. I was working on the same solution, but i've also try to
remove x86emu (not possible, used even on i386).

Patch applied and send upstream.

Test on FC5.i386 (my last i386 available OS) : run well.

Scratch build : http://koji.fedoraproject.org/koji/taskinfo?taskID=551901

Spec URL: http://remi.fedorapeople.org/monitor-edid.spec
SRPM URL: http://remi.fedorapeople.org/monitor-edid-1.16-4.fc8.src.rpm
Comment 9 Ville Skyttä 2008-04-06 15:12:20 EDT

There's no need to %ifarch the removal of lrmi.h and lrmi.c though, you can do
that for all arches for clarity.
Comment 10 Remi Collet 2008-04-06 16:07:16 EDT
Thanks for the review.

New Package CVS Request
Package Name: monitor-edid
Short Description: Tool for probing and parsing EDID
Owners: remi
Branches: F-7, F-8, EL-5, EL-4
Cvsextras Commits: yes
Comment 11 Kevin Fenzi 2008-04-06 23:59:41 EDT
cvs done.

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