Review Request 110091: clean up and update FreeBSD support for kinfocenter

[Max Brazhnikov]

> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > cmake/modules/FindLibdevinfo.cmake, lines 8-11
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139990#file139990line8>
> >
> >     Why is this necessary at all? If you don't have a strong reason for it just drop it.

result of copy-paste. Removed.


> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > kinfocenter/Modules/base/CMakeLists.txt, line 5
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139991#file139991line5>
> >
> >     The TODO comment above can go away now I think.

TODO is about Solaris. It has devinfo library, which is incompatible with FreeBSD realization and requires different header (libdevinfo.h vs FreeBSD's devinfo.h)


> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > kinfocenter/Modules/base/info_fbsd.cpp, line 136
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139992#file139992line136>
> >
> >     Why not just use QProcess here to get the result? I fear this stuff dates back to QT(<=3) times where this probably had issues, but that isn't true anymore.

GetInfo_ReadfromPipe already uses QProcess.


> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > kinfocenter/Modules/base/info_fbsd.cpp, line 168
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139992#file139992line168>
> >
> >     QProcess here, too. Otherwise it may be a good idea to just put your patch in now as it is and do a followup patch that just kills that function and replaces it with QProcess, or keeps that functions and gives it a proper QProcess-based interface, i.e. separate arguments passed as a QStringList and such.

GetInfo_ReadfromPipe is also used in info_hpux.cpp and info_linux.cpp, so I'd keep it.


> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > cmake/modules/FindLibdevinfo.cmake, line 16
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139990#file139990line16>
> >
> >     Has the header a version number one could parse out and use?
> >

devinfo is a part of FreeBSD base distribution and has no verion. However it's been ported to DragonFly, so it's not FreeBSD specific now.


> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > kinfocenter/Modules/info/CMakeLists.txt, line 15
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139993#file139993line15>
> >
> >     I miss a include_directories(${DEVINFO_INCLUDE_DIRS}) or something like that here.

it's not really required, because devinfo.h is always installed to /usr/include


> On April 20, 2013, 2:11 p.m., Rolf Eike Beer wrote:
> > cmake/modules/FindLibdevinfo.cmake, line 12
> > <http://git.reviewboard.kde.org/r/110091/diff/1/?file=139990#file139990line12>
> >
> >     Please read Modules/readme.txt from CMake about how to name such variables. Short: this should be DEVINFO_INCLUDE_DIR, and you should have a DEVINFO_INCLUDE_DIRS later, which is not cached.

thanks for the hint, done.


- Max


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110091/#review31332
-----------------------------------------------------------


On April 30, 2013, 11:50 a.m., Max Brazhnikov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110091/
> -----------------------------------------------------------
> 
> (Updated April 30, 2013, 11:50 a.m.)
> 
> 
> Review request for kde-workspace.
> 
> 
> Description
> -------
> 
> Add FindLibdevinfo.cmake
> Clean up info_fbsd.cpp and utilize PCIUtils
> 
> 
> Diffs
> -----
> 
>   cmake/modules/FindLibdevinfo.cmake PRE-CREATION 
>   kinfocenter/Modules/base/CMakeLists.txt 2b3c34e 
>   kinfocenter/Modules/base/info_fbsd.cpp 6bbaa1a 
>   kinfocenter/Modules/info/CMakeLists.txt dba6bc7 
> 
> Diff: http://git.reviewboard.kde.org/r/110091/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Max Brazhnikov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130430/7ee3cfc0/attachment.htm>