Bug 251019 - Review Request: lshw - Hardware lister
Review Request: lshw - Hardware lister
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Extras Quality Assurance
:
: 229591 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-06 11:20 EDT by Terje Røsten
Modified: 2012-02-06 08:10 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-15 17:38:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wolfy: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Terje Røsten 2007-08-06 11:20:05 EDT
SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
SRPM: http://terjeros.fedorapeople.org/lshw/lshw-B.02.11.01-1.fc7.src.rpm
Description:

lshw is a small tool to provide detailed informaton on the hardware
configuration of the machine. It can report exact memory configuration,
firmware version, mainboard configuration, CPU version and speed, cache
configuration, bus speed, etc. on DMI-capable x86 systems and on some
PowerPC machines (PowerMac G4 is known to work).

Add GUI frontend is also included in a subpackage.
Comment 1 Terje Røsten 2007-08-06 11:20:51 EDT
*** Bug 229591 has been marked as a duplicate of this bug. ***
Comment 2 David Timms 2007-08-06 19:07:33 EDT
Note: The bug "duplicate"d tracks the original submitters requests of upstream
to make fedora specific changes to the upstream spec. Terje has since picked up
this package, and the initial discussions and changes to the package are
documented in the duplicated bug.
---
Re: SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
There is something a bit weird about the versions: the upstream Changelog log is
calling this B.02.12, while the src and this spec, web site call it B.02.11.01.
That's probably just an oversight.
---
From:
http://fedoraproject.org/wiki/PackageReviewProcess#head-f3a689ab03b01268036d0533d25e2fcfba3f38ea
"If the package is legally risky for whatever reason (known patent or copyright
infringement, trademark concerns) close the bug WONTFIX and leave an appropriate
comment (i.e. we don't ship mp3, so stop submitting it). Set the fedora-review
flag to -, and have the review ticket block FE-Legal."

For an external trademarked logo, I believe you would have to get specific
permission to use the logo in other settings/software. I don't think it would
matter whether the image is an original from the owner of the trademark or
recreated by an upstream project, it would still be content that can't be included.

I'll ask on f-devel-l whether such logos would be permissible content.

Lionel: if we aren't able to use other entities' trademarks, can lshw-gui
operate if it can't find these images ? Would you be prepared to provide
alternative non-trademarked content - eg an artistic rendering like the images
of Apple boxes ?
---
I notice the the installed location is now /sbin. Is it the intent that gtk-lshw
not be called through consolehelper, but lshw-gui is ? {I haven't experience in
this area}.
Comment 3 Lyonel Vincent 2007-08-06 20:33:06 EDT
(In reply to comment #2)
> Note: The bug "duplicate"d tracks the original submitters requests of upstream
> to make fedora specific changes to the upstream spec. Terje has since picked up
> this package, and the initial discussions and changes to the package are
> documented in the duplicated bug.
> ---
> Re: SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
> There is something a bit weird about the versions: the upstream Changelog log is
> calling this B.02.12, while the src and this spec, web site call it B.02.11.01.
> That's probably just an oversight.

yes, my mistake.

> ---
> From:
>
http://fedoraproject.org/wiki/PackageReviewProcess#head-f3a689ab03b01268036d0533d25e2fcfba3f38ea
> "If the package is legally risky for whatever reason (known patent or copyright
> infringement, trademark concerns) close the bug WONTFIX and leave an appropriate
> comment (i.e. we don't ship mp3, so stop submitting it). Set the fedora-review
> flag to -, and have the review ticket block FE-Legal."
> 
> For an external trademarked logo, I believe you would have to get specific
> permission to use the logo in other settings/software. I don't think it would
> matter whether the image is an original from the owner of the trademark or
> recreated by an upstream project, it would still be content that can't be
included.
> 
> I'll ask on f-devel-l whether such logos would be permissible content.

> Lyonel: if we aren't able to use other entities' trademarks, can lshw-gui
> operate if it can't find these images ? Would you be prepared to provide
> alternative non-trademarked content - eg an artistic rendering like the images
> of Apple boxes ?

well... there isn't much to do with USB/SCSI/firewire logos :-) (I don't think
these ones can be problematic, AMD/Intel/PowerPC might be)
in any case, lshw-gui can of course operate without the logos

> ---
> I notice the the installed location is now /sbin. Is it the intent that gtk-lshw
> not be called through consolehelper, but lshw-gui is ? {I haven't experience in
> this area}.

 * I think there is a mixup in the installation section: make install
SBINDIR=/usr/sbin doesn't do anything.
 * shouldn't /usr/sbin be replaced by some %{value} (in .consolehelper)?
 * shouldn't /usr/bin be removed from .desktop launcher?

Comment 4 Terje Røsten 2007-08-07 02:27:51 EDT
>  * I think there is a mixup in the installation section: make install
> SBINDIR=/usr/sbin doesn't do anything.

Well, bins are installed in /usr/sbin

>  * shouldn't /usr/sbin be replaced by some %{value} (in .consolehelper)?

As %{_sbin}/%{name}-qui ?

>  * shouldn't /usr/bin be removed from .desktop launcher?

Why? I want to launch /usr/bin/lshw-gui, not anything else.


>I notice the the installed location is now /sbin. 

Yes, Lyonel  said lshw* had to run as root, then they belong in /usr/sbin,
the consolehelper is of course in /usr/bin, see consolehelper(8) .

> Is it the intent that  gtk-lshw not be called through consolehelper, but
> lshw-gui is ?  {I haven't experienc in  this area}.

A choice I made, the result is that normal users will not see the
gtk-lshw command, only lshw-gui. Don't a big problem?




Comment 5 Lyonel Vincent 2007-08-07 03:05:51 EDT
(In reply to comment #4)
> >  * I think there is a mixup in the installation section: make install
> > SBINDIR=/usr/sbin doesn't do anything.
> 
> Well, bins are installed in /usr/sbin
>
> >  * shouldn't /usr/sbin be replaced by some %{value} (in .consolehelper)?
> 
> As %{_sbin}/%{name}-gui ?

yes, or %{_sbindir}/gtk-lshw for example

> >  * shouldn't /usr/bin be removed from .desktop launcher?
> 
> Why? I want to launch /usr/bin/lshw-gui, not anything else.

I noticed that (most of?) system-config*.desktop files don't honour $PATH but
other applications (gimp-2.2.desktop, etc.) do.
Is there a rule of thumb for this?
 
> >I notice the the installed location is now /sbin. 
> 
> Yes, Lyonel  said lshw* had to run as root, then they belong in /usr/sbin,
> the consolehelper is of course in /usr/bin, see consolehelper(8) .
> 
> > Is it the intent that  gtk-lshw not be called through consolehelper, but
> > lshw-gui is ?  {I haven't experienc in  this area}.
> 
> A choice I made, the result is that normal users will not see the
> gtk-lshw command, only lshw-gui. Don't a big problem?
> 
> 

Comment 6 Terje Røsten 2007-08-07 12:54:41 EDT
spot, fedora's license guru has spoken:

 http://article.gmane.org/gmane.linux.redhat.fedora.devel/60831

I will rm -f the in svgs that can't be shipped.

Would of course be nice to have those replaced, however I don't see
this as blocker for inclusion in fedora (as long the app will work
without the svgs.)

Thanks for the help, David.
Comment 7 Terje Røsten 2007-08-07 17:03:28 EDT
After talking with spot I have removed some logos, package should be safe now.

lshw-gui seems to work ok without the logos.

SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
SRPM: http://terjeros.fedorapeople.org/lshw/lshw-B.02.11.01-2.fc7.src.rpm
Comment 8 David Timms 2007-08-11 10:15:09 EDT
(In reply to comment #7)
> After talking with spot I have removed some logos, package should be safe now.
> 
> lshw-gui seems to work ok without the logos.
> 
> SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
> SRPM: http://terjeros.fedorapeople.org/lshw/lshw-B.02.11.01-2.fc7.src.rpm

In terms of the requiring admin privileges to run: lshw can run as a normal user
- if so, only hardware information available as a normal user will be shown.
More information is show if run as root {or through consolehelper}.

Pre-review of lshw-B.02.11.01-2.fc7.src.rpm:
===== key:
.x = Not ok
.? = Either I don't understand enough to confirm that the element is acceptable,
or I don't understand what the spec is doing. I would request some comment to
explain if you believe that the item is OK.
./ = tick - OK. No work required. Usually a comment was included after: to
describe what I found.
=====
MUST Items:
./ rpmlint result: no warnings nor errors.
./ named according to the Package Naming Guidelines: matches upstream project
and source download name.
./ spec file name matches the base package: lshw.spec lshw 
.? package must meet the Packaging Guidelines.
./ package must be licensed with an open-source compatible license:
  - web site indicates GPL and upstream source includes GPLv2.
./ License field in the package spec file must match the actual license:
  - GPLv2 as required by new licensing guideline
./ source package includes the text of the license, so text of the license(s)
for the package must be included in %doc:
  - COPYING is included as required.
  - 6* Trademark svg graphics are included in the source tarball, but the spec
removes them. Discussion on fedora-devel and with spot lead to this requirement.
  - build log shows successful removal:
+ for f in powermacg5 intel powermac amd mini powerpc
+ rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermacg5.svg
removed
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermacg5.svg'
+ for f in powermacg5 intel powermac amd mini powerpc
+ rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/intel.svg
removed
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/intel.svg'
+ for f in powermacg5 intel powermac amd mini powerpc
+ rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermac.svg
removed
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermac.svg'
+ for f in powermacg5 intel powermac amd mini powerpc
+ rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/amd.svg
removed
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/amd.svg'
+ for f in powermacg5 intel powermac amd mini powerpc
+ rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/mini.svg
removed
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/mini.svg'
+ for f in powermacg5 intel powermac amd mini powerpc
+ rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powerpc.svg
removed
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powerpc.svg'  

  - emac.svg {artistic view of apple logo} is still included - is it OK to
include in Fedora ?
  
./ The spec file must be written in American English.
./ spec file for the package must be legible:
./ source in .src.rpm matches upstream md5sum:
  - $ md5sum lshw-B.02.11.01.tar.gz
23debbc3c0a719f301861cfc079b3f4b  lshw-B.02.11.01.tar.gz  {web source}
23debbc3c0a719f301861cfc079b3f4b  /usr/src/redhat/SOURCES/lshw-B.02.11.01.tar.gz

./ successfully compiles and builds into binary rpms: i386 {athlon}
.? If the package does not successfully compile, build or work on an
architecture - :
  - I tried only on i386{i686/athlon}
  - no excludearchs listed.
  - have you tested on x86_64 or mac ?

./ build dependencies must be listed in BuildRequires:
  - no listed BR is in the auto included list
  - the package built on my system
  - package built in mock
  - built package runs OK, works as expected
./ spec file MUST handle locales properly:
  - neither find_lang macro nor %{_datadir}/locale are used.
./ has no shared library files: 
  - contains only standalone executables with external text lookup files
./ not relocatable and does not use Prefix: /usr
./ package must own all directories that it creates:
  - 
./ A package must not contain any duplicate files in the %files listing:
  - does not appear to.
./ Permissions on files must be set properly. 
  - Executables are set with executable permissions
  - other files aren't
  - %files section includes a %defattr(...) line.
.x must have a %clean section, containing rm -rf %{buildroot} (or $RPM_BUILD_ROOT):
  - %{__rm} -rf %{buildroot} differs from this requirement.
  
.? Each package must consistently use macros:
  - no new macros are defined.
  - %{x} are actually %{__X}, again, why ?
  
./ The package must contain code, or permissable content.
  - contains a GUI app
./ Large documentation files:
  - no. total doc is 43kB
? - COPYING is included in both lshw and lshw-gui. However, lshw-gui Requires 
lshw, so the COPYING is guaranteed to be installed already. I don't know if
that is normal, or whether it could / should be removed.

./ %doc files must not affect the runtime of the application:
  - OK.
./ Header files must be in a -devel package:
  - no header files.
./ Static libraries must be in a -static package:
  - no static libraries.
./ has no pkgconfig(.pc) files.
./ library files with a suffix: no libraries
./ devel packages must require the base package: 
  - no -devel package
./ Packages must NOT contain any .la libtool archives
  - no .la's
./ Packages containing GUI applications must include a %{name}.desktop file, and
that file must be properly installed with desktop-file-install
  - done and the menu entry works.
./ Packages must not own files or directories already owned by other packages. 
.x At the beginning of %install, each package MUST run rm -rf %{buildroot} (or
$RPM_BUILD_ROOT):
  - the command is: %{__rm} -rf %{buildroot}, which is not standard, why ?
  
./ All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
./ If the source package does not include license text(s) as a separate file
from upstream, the packager SHOULD query upstream to include it: included.
./ The description and summary sections in the package spec file should contain
translations for supported Non-English languages:
  - no other translations available 
.?  The package should compile and build into binary rpms on all supported
architectures:
  - only tested on i386 {athlon}  
  
./ package functions as described
  - both GUI and CLI provide a lot of hw information as the summary describes
./  If scriptlets are used, those scriptlets must be sane: 
  - none used.
./  subpackages other than devel should require the base package using a
fully versioned dependency.
./ no pkgconfig(.pc) files.
./ no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin.
=====
Personally, I think console helper should be saying ~"this program performs
better when run with root privileges", and give the normal user a chance to ~run
 unprivileged". This makes the app at least somewhat useful if you do not have
root rights on the system. This might also means not placing in /usr/sbin ?

I also suggest: that even though lshw includes a copy of hw data, that any such
files included by fedora hwdata should not be packaged. This has the advantages of:
- removes any confusion / discrepancy that would otherwise occur if the result
of other fedora hardware tools is compared to lshw that included different
hwdata files.
- shouldn't require updates to lshw when only hw data has been updated.

disadvantages:
- fedora's hwdata does not seem to be updated very often - but perhaps it should
get updated say once a month or so to take into account newly released hardware.
- if lshw is updated {upstream updates all hw data before a release}, the fedora
lshw would not be able to immediately use new data. This is countered by the
fact that a package should be essentially static during a distribution's release
life. If the main change is to hw data - then only a new hwdata should be released.

What do others think ?
Comment 9 David Timms 2007-08-11 10:18:23 EDT
ps: Lyonel, I noticed I mispelled your name, my apologies.
Comment 10 Lyonel Vincent 2007-08-11 12:56:24 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > After talking with spot I have removed some logos, package should be safe now.
> > 
> > lshw-gui seems to work ok without the logos.
> > 
> > SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
> > SRPM: http://terjeros.fedorapeople.org/lshw/lshw-B.02.11.01-2.fc7.src.rpm
> 
> In terms of the requiring admin privileges to run: lshw can run as a normal user
> - if so, only hardware information available as a normal user will be shown.
> More information is show if run as root {or through consolehelper}.

The problem is that the reported information will be highly inaccurate (and, on
certain platforms like x86, not only incomplete but *different* from what you
get as root) when lshw is run as normal user. Maybe I should add a warning in
the GUI (like what you get when running the CLI as non-root user). What do
others think?
 
> Pre-review of lshw-B.02.11.01-2.fc7.src.rpm:
> ===== key:
> .x = Not ok
> .? = Either I don't understand enough to confirm that the element is acceptable,
> or I don't understand what the spec is doing. I would request some comment to
> explain if you believe that the item is OK.
> ./ = tick - OK. No work required. Usually a comment was included after: to
> describe what I found.
> =====
> MUST Items:
> ./ rpmlint result: no warnings nor errors.
> ./ named according to the Package Naming Guidelines: matches upstream project
> and source download name.
> ./ spec file name matches the base package: lshw.spec lshw 
> .? package must meet the Packaging Guidelines.
> ./ package must be licensed with an open-source compatible license:
>   - web site indicates GPL and upstream source includes GPLv2.
> ./ License field in the package spec file must match the actual license:
>   - GPLv2 as required by new licensing guideline
> ./ source package includes the text of the license, so text of the license(s)
> for the package must be included in %doc:
>   - COPYING is included as required.
>   - 6* Trademark svg graphics are included in the source tarball, but the spec
> removes them. Discussion on fedora-devel and with spot lead to this requirement.
>   - build log shows successful removal:
> + for f in powermacg5 intel powermac amd mini powerpc
> + rm -fv
/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermacg5.svg
> removed
>
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermacg5.svg'
> + for f in powermacg5 intel powermac amd mini powerpc
> + rm -fv
> /var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/intel.svg
> removed
> `/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/intel.svg'
> + for f in powermacg5 intel powermac amd mini powerpc
> + rm -fv
> /var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermac.svg
> removed
>
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powermac.svg'
> + for f in powermacg5 intel powermac amd mini powerpc
> + rm -fv
> /var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/amd.svg
> removed
> `/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/amd.svg'
> + for f in powermacg5 intel powermac amd mini powerpc
> + rm -fv
> /var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/mini.svg
> removed
> `/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/mini.svg'
> + for f in powermacg5 intel powermac amd mini powerpc
> + rm -fv
> /var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powerpc.svg
> removed
>
`/var/tmp/lshw-B.02.11.01-2.fc7-root-mockbuild/usr/share/lshw/artwork/powerpc.svg'  

I have added to lshw's SVN some drawings without logos and also "replacement"
logos for intel, amd, powerpc (cf.
http://dev.ezix.org/source/packages/artwork/nologo/ comments welcome).

>   - emac.svg {artistic view of apple logo} is still included - is it OK to
> include in Fedora ?
>   
> ./ The spec file must be written in American English.
> ./ spec file for the package must be legible:
> ./ source in .src.rpm matches upstream md5sum:
>   - $ md5sum lshw-B.02.11.01.tar.gz
> 23debbc3c0a719f301861cfc079b3f4b  lshw-B.02.11.01.tar.gz  {web source}
> 23debbc3c0a719f301861cfc079b3f4b  /usr/src/redhat/SOURCES/lshw-B.02.11.01.tar.gz
> 
> ./ successfully compiles and builds into binary rpms: i386 {athlon}
> .? If the package does not successfully compile, build or work on an
> architecture - :
>   - I tried only on i386{i686/athlon}
>   - no excludearchs listed.
>   - have you tested on x86_64 or mac ?

ppc is OK, that's my test/dev machine.
 
> ./ build dependencies must be listed in BuildRequires:
>   - no listed BR is in the auto included list
>   - the package built on my system
>   - package built in mock
>   - built package runs OK, works as expected
> ./ spec file MUST handle locales properly:
>   - neither find_lang macro nor %{_datadir}/locale are used.
> ./ has no shared library files: 
>   - contains only standalone executables with external text lookup files
> ./ not relocatable and does not use Prefix: /usr
> ./ package must own all directories that it creates:
>   - 
> ./ A package must not contain any duplicate files in the %files listing:
>   - does not appear to.
> ./ Permissions on files must be set properly. 
>   - Executables are set with executable permissions
>   - other files aren't
>   - %files section includes a %defattr(...) line.
> .x must have a %clean section, containing rm -rf %{buildroot} (or
$RPM_BUILD_ROOT):
>   - %{__rm} -rf %{buildroot} differs from this requirement.
>   
> .? Each package must consistently use macros:
>   - no new macros are defined.
>   - %{x} are actually %{__X}, again, why ?
>   
> ./ The package must contain code, or permissable content.
>   - contains a GUI app
> ./ Large documentation files:
>   - no. total doc is 43kB
> ? - COPYING is included in both lshw and lshw-gui. However, lshw-gui Requires 
> lshw, so the COPYING is guaranteed to be installed already. I don't know if
> that is normal, or whether it could / should be removed.
> 
> ./ %doc files must not affect the runtime of the application:
>   - OK.
> ./ Header files must be in a -devel package:
>   - no header files.
> ./ Static libraries must be in a -static package:
>   - no static libraries.
> ./ has no pkgconfig(.pc) files.
> ./ library files with a suffix: no libraries
> ./ devel packages must require the base package: 
>   - no -devel package
> ./ Packages must NOT contain any .la libtool archives
>   - no .la's
> ./ Packages containing GUI applications must include a %{name}.desktop file, and
> that file must be properly installed with desktop-file-install
>   - done and the menu entry works.
> ./ Packages must not own files or directories already owned by other packages. 
> .x At the beginning of %install, each package MUST run rm -rf %{buildroot} (or
> $RPM_BUILD_ROOT):
>   - the command is: %{__rm} -rf %{buildroot}, which is not standard, why ?
>   
> ./ All filenames in rpm packages must be valid UTF-8.
> 
> SHOULD Items:
> ./ If the source package does not include license text(s) as a separate file
> from upstream, the packager SHOULD query upstream to include it: included.
> ./ The description and summary sections in the package spec file should contain
> translations for supported Non-English languages:
>   - no other translations available 
> .?  The package should compile and build into binary rpms on all supported
> architectures:
>   - only tested on i386 {athlon}  
>   
> ./ package functions as described
>   - both GUI and CLI provide a lot of hw information as the summary describes
> ./  If scriptlets are used, those scriptlets must be sane: 
>   - none used.
> ./  subpackages other than devel should require the base package using a
> fully versioned dependency.
> ./ no pkgconfig(.pc) files.
> ./ no file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin.
> =====
> Personally, I think console helper should be saying ~"this program performs
> better when run with root privileges", and give the normal user a chance to ~run
>  unprivileged". This makes the app at least somewhat useful if you do not have
> root rights on the system. This might also means not placing in /usr/sbin ?
> 
> I also suggest: that even though lshw includes a copy of hw data, that any such
> files included by fedora hwdata should not be packaged. This has the
advantages of:
> - removes any confusion / discrepancy that would otherwise occur if the result
> of other fedora hardware tools is compared to lshw that included different
> hwdata files.
> - shouldn't require updates to lshw when only hw data has been updated.
> 
> disadvantages:
> - fedora's hwdata does not seem to be updated very often - but perhaps it should
> get updated say once a month or so to take into account newly released hardware.
> - if lshw is updated {upstream updates all hw data before a release}, the fedora
> lshw would not be able to immediately use new data. This is countered by the
> fact that a package should be essentially static during a distribution's release
> life. If the main change is to hw data - then only a new hwdata should be
released.
> 
> What do others think ?

as discussed earlier (cf. duplicate BR), lshw makes use of *both* sources
(system's hwdata and bundled data files) so it always uses the most recent data.
 It'd be good to have Fedora's hwdata updated once a month but it's currently
very old and updated infrequently. I may add an option (in the GUI) to download
fresher files in the future and cache them in $XDG_CACHE_HOME/lshw (usually
$HOME/.cache/lshw).

Comment 11 manuel wolfshant 2007-08-11 16:42:28 EDT
lshw-B.02.11.01-2.fc7.src.rpm builds fine in devel/x86_64. rpmlint is silent on
all generated rpm files:
[wolfy@wolfy64 ~]$ mock -r fedora-devel-x86_64 lshw-B.02.11.01-2.fc7.src.rpm
[...]Results and/or logs in: /var/lib/mock/fedora-development-x86_64/result
[wolfy@wolfy64 ~]$ ll /var/lib/mock/fedora-development-x86_64/result
total 4560
-rw-r--r-- 1 wolfy mock   26224 2007-08-11 23:30 build.log
-rw-rw-r-- 1 wolfy mock 1169217 2007-08-11 23:28 lshw-B.02.11.01-2.fc8.src.rpm
-rw-rw-r-- 1 wolfy mock 1145047 2007-08-11 23:30 lshw-B.02.11.01-2.fc8.x86_64.rpm
-rw-rw-r-- 1 wolfy mock 1844959 2007-08-11 23:30
lshw-debuginfo-B.02.11.01-2.fc8.x86_64.rpm
-rw-rw-r-- 1 wolfy mock  402023 2007-08-11 23:30
lshw-gui-B.02.11.01-2.fc8.x86_64.rpm
-rw-r--r-- 1 wolfy mock     171 2007-08-11 23:26 mockconfig.log
-rw-r--r-- 1 wolfy mock   20131 2007-08-11 23:30 root.log
[wolfy@wolfy64 ~]$ rpmlint /var/lib/mock/fedora-development-x86_64/result/*rpm
Comment 12 David Timms 2007-08-11 20:56:28 EDT
(In reply to comment #10)
> I have added to lshw's SVN some drawings without logos and also "replacement"
> logos for intel, amd, powerpc (cf.
> http://dev.ezix.org/source/packages/artwork/nologo/ comments welcome).
Perusing the nologo images, I find that actual trademarks have been replaced
with chip-like icons with a plain font describing the type {alpha /amx /intel
/mips /pa-risc /powerpc /sparc} and the boxen graphics show the form of the
apple boxes, without using a logo.

I would say these are acceptable for inclusion in Fedora. Lyonel, will these be
included in a future source release ?
Comment 13 Terje Røsten 2007-08-13 14:50:18 EDT
> MUST Items:
>  - emac.svg {artistic view of apple logo} is still included - is it OK to
> include in Fedora ?

Logo? It's a drawing of a computer monitor, must be ok.


> .? If the package does not successfully compile, build or work on an
> architecture - :
>  - I tried only on i386{i686/athlon}
>  - no excludearchs listed.
>  - have you tested on x86_64 or mac ?

Lyonel on ppc, I have tested x86_64, ok.

> .x At the beginning of %install, each package MUST run rm -rf %{buildroot} (or
> $RPM_BUILD_ROOT):
>  - the command is: %{__rm} -rf %{buildroot}, which is not standard, why ?
>
>.x must have a %clean section, containing rm -rf %{buildroot} (or $RPM_BUILD_ROOT):
> - %{__rm} -rf %{buildroot} differs from this requirement.

Not fixed, see next issue.
  
>.? Each package must consistently use macros:
>  - no new macros are defined.
>  - %{x} are actually %{__X}, again, why ?

I read this as not to mix $RPM_BUILD_ROOT and %{buildroot}, sed and %{__sed}.
I prefer to use the macro versions, hence rm -rf is %{__rm} -rf.
Anyway, this is pedantic.


> ? - COPYING is included in both lshw and lshw-gui. However, lshw-gui Requires 
> lshw, so the COPYING is guaranteed to be installed already. I don't know if
> that is normal, or whether it could / should be removed.

Requires is not enough, lshw-gui could e.g have a BSD license.

  
> .?  The package should compile and build into binary rpms on all supported
> architectures:
>  - only tested on i386 {athlon}

Tested on i386, ppc and x86_64.


> Personally, I think console helper should be saying ~"this program performs
> better when run with root privileges", and give the normal user a chance to ~run
> unprivileged". This makes the app at least somewhat useful if you do not have
> root rights on the system. This might also means not placing in /usr/sbin ?

This not make sense to me.


> I also suggest: that even though lshw includes a copy of hw data, that any such
> files included by fedora hwdata should not be packaged. This has the
advantages of:
> - removes any confusion / discrepancy that would otherwise occur if the result
> of other fedora hardware tools is compared to lshw that included different
> hwdata files.
> - shouldn't require updates to lshw when only hw data has been updated.

Until hwdata is more frequently updated the way lshw is doing this now
is most sane, if things changes hwdata info in lshw can be removed.

Comment 14 Terje Røsten 2007-08-13 15:04:11 EDT
> The problem is that the reported information will be highly inaccurate (and, on
> certain platforms like x86, not only incomplete but *different* from what you
> get as root) when lshw is run as normal user.

Agree, you should be root to run the tool consolehelper solves this.


> Maybe I should add a warning in
> the GUI (like what you get when running the CLI as non-root user). What do
> others think?

A warning would be nice, however that's is feature request in lshw, is
not critical for this review which is about inclusion in Fedora (let's
not forget that).


> I have added to lshw's SVN some drawings without logos and also "replacement"
> logos for intel, amd, powerpc (cf.
> http://dev.ezix.org/source/packages/artwork/nologo/ comments welcome).

Looks great, will include them when they are part of a proper
release. Again: lshw works great without logs, this issue is not
critical for this Fedora inclusion review.


> as discussed earlier (cf. duplicate BR), lshw makes use of *both* sources
> (system's hwdata and bundled data files) so it always uses the most recent data.
> It'd be good to have Fedora's hwdata updated once a month but it's currently
> very old and updated infrequently.

In a perfect world hwdata would be updated every month, it's not so
lshw must be included it's own data. Things might change and then we
will adapt.


> I may add an option (in the GUI) to download fresher files in the
> future and cache them in $XDG_CACHE_HOME/lshw (usually
> $HOME/.cache/lshw).

Nothing stop you, it's your software, however it outside the scope of
this review.
Comment 15 manuel wolfshant 2007-08-13 18:48:44 EDT
Official review (not final yet!)
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on:all available archs, see
http://koji.fedoraproject.org/koji/taskinfo?taskID=101050
 [x] Rpmlint output: no output
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type: GPLv2 [see issue 1]
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [?] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     SHA1SUM of package: See issue 2
 [x] Package is not known to require ExcludeArch, OR:
     Arches excluded:
     Why:
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [x] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [?] Latest version is packaged. See issue 3
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on:http://koji.fedoraproject.org/koji/taskinfo?taskID=101050 :
devel/ i386, x86_64, ppc, ppc64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:devel/i386, x86_64, ppc, ppc64
 [?] Package functions as described. See issue 4
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [-] File based requires are sane.

=== Issues ===
1. I am not sure which license is used by the program. The Copying file mentions
the GPLv2 or later clauses,which would mean that the correct tag is GPLv2+.
However I might misinterpretate it, so Lyonel, please assist and let us know
what is the intended licensing scheme.
2. The source URL included was not accesible, the site seemed to be down at the
moment of the review
3. The file named "Changes" references a later version of the source. As the
site for upstream was not accessible, I could not check
4. I will verify this a bit later.
5. Please use separate files for the pam and desktop files. Creating them in the
spec is technically OK, but is prone to errors, while using separate files
allows better time/version/MD5 checking
6. (pedantic, feel free to ignore): Please verify if "make" and "make gui" are
both needed. I cannot test now, but at the first glance I think that "make gui"
is enough, it will build everything.

=== Final Notes ===
The package is in excellent shape. Once we clarify the license tag and I can
verify against upstream sources, I will do a test run and approve. I would
appreciate if meanwhile you would implement a solution to issue 5 above.
Comment 16 Terje Røsten 2007-08-14 13:05:27 EDT
> === Issues ===
> 1. I am not sure which license is used by the program. The Copying file mentions
> the GPLv2 or later clauses,which would mean that the correct tag is GPLv2+.
> However I might misinterpretate it, so Lyonel, please assist and let us know
> what is the intended licensing scheme.

GPLv2 is correct, ref:

 https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=229591#c42

> 2. The source URL included was not accesible, the site seemed to be down at the
> moment of the review
> 3. The file named "Changes" references a later version of the source. As the
> site for upstream was not accessible, I could not check

That is a known issue, ref: comment #3 over.
 

> 4. I will verify this a bit later.
> 5. Please use separate files for the pam and desktop files. Creating them in the
> spec is technically OK, but is prone to errors, while using separate files
> allows better time/version/MD5 checking

Fixed.

> 6. (pedantic, feel free to ignore): Please verify if "make" and "make gui" are
> both needed. I cannot test now, but at the first glance I think that "make gui"
> is enough, it will build everything.

You are right, fixed.
 
Updated version:
SPEC: http://terjeros.fedorapeople.org/lshw/lshw.spec
SRPM: http://terjeros.fedorapeople.org/lshw/lshw-B.02.11.01-3.fc7.src.rpm
Comment 17 manuel wolfshant 2007-08-15 06:40:35 EDT
The log of the mock build includes the following lines:
 Processing files: lshw-debuginfo-B.02.11.01-3.fc8
 warning: File listed twice: /usr/src/debug/lshw-B.02.11.01
 warning: File listed twice: /usr/src/debug/lshw-B.02.11.01/src
 warning: File listed twice: /usr/src/debug/lshw-B.02.11.01/src/core
 warning: File listed twice: /usr/src/debug/lshw-B.02.11.01/src/gui
This looks like a bug to me, so maybe you should report it.

On with the rest of the review
- source matches upstream, sha1sum is 
3a7e790b2912abb5d896a2de21c594009be52d78 lshw-B.02.11.01.tar.gz
- license tag is OK
- all other questions were properly answered.

Package APPROVED


Comment 18 Terje Røsten 2007-08-15 13:23:01 EDT
> Package APPROVED

Thanks for the review manuel!

Thanks to other guys too, and of course to a very helpful upstream maintainer!


 - Terje


New Package CVS Request
=======================
Package Name: lshw
Short Description: Hardware lister
Owners: terjeros
Branches: FC-6 F-7
InitialCC:
Cvsextras Commits: yes
Comment 19 Kevin Fenzi 2007-08-15 14:02:51 EDT
cvs done. 
Note that your License tag should be: GPLv2+ if it's really GPL version 2 or later. 

Comment 20 Terje Røsten 2007-08-15 17:38:39 EDT
> Note that your License tag should be: GPLv2+ if it's really GPL version 2 or
later. 


It's GPLv2.

Bodhi done. Closing.
Comment 21 Steve Traylen 2012-02-05 06:13:14 EST
Package Change Request
======================
Package Name: lshw
New Branches: el6
Owners: stevetraylen

From #781486 

Ok, great, I don't maintain EPEL branches for any package.
Comment 22 Jon Ciesla 2012-02-06 08:10:14 EST
Already has EL-6 branch, to add owners see pkgdb.

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