Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=437691
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: fedora@famillecollet.com QAContact: extras-qa@fedoraproject.org CC: fedora-package-review@redhat.com,notting@redhat.com
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
Description: 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |ville.skytta@iki.fi Status|NEW |ASSIGNED Flag| |fedora-review?
------- Additional Comments From ville.skytta@iki.fi 2008-03-16 15:24 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From fedora@famillecollet.com 2008-03-16 16:11 EST ------- 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).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From notting@redhat.com 2008-03-17 11:32 EST ------- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From ville.skytta@iki.fi 2008-03-17 16:16 EST ------- (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 http://fedoraproject.org/wiki/Licensing
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From fedora@famillecollet.com 2008-03-17 16:41 EST ------- @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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From fedora@famillecollet.com 2008-04-05 14:37 EST ------- @Ville Skyttä is there is still a problem with this package ?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From ville.skytta@iki.fi 2008-04-05 16:58 EST ------- Created an attachment (id=301389) --> (https://bugzilla.redhat.com/attachment.cgi?id=301389&action=view) 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 http://en.wikipedia.org/wiki/EDID
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
------- Additional Comments From fedora@famillecollet.com 2008-04-06 03:42 EST ------- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
ville.skytta@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
------- Additional Comments From ville.skytta@iki.fi 2008-04-06 15:12 EST ------- Approved.
There's no need to %ifarch the removal of lrmi.h and lrmi.c though, you can do that for all arches for clarity.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
fedora@famillecollet.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
------- Additional Comments From fedora@famillecollet.com 2008-04-06 16:07 EST ------- 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 InitialCC: Cvsextras Commits: yes
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
------- Additional Comments From kevin@tummy.com 2008-04-06 23:59 EST ------- cvs done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: monitor-edid - Tool for probing and parsing EDID
https://bugzilla.redhat.com/show_bug.cgi?id=437691
fedora@famillecollet.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla