> 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 > >