Re: Review Request 110271: libusb-1 support in kcmusb (kinfocenter)
On May 12, 2013, 9:21 a.m., Alexander Neundorf wrote: There is a Find-module for libusb in extra-cmake-modules. As long as this is not released yet, please use a copy from the one over there: http://quickgit.kde.org/?p=extra-cmake-modules.gita=blobh=eba47115f94cc2eb969f19166d6664b300864a30hb=316c7756ec1b0745f518d2f9af139812c3c5a24ff=find-modules%2FFindLibUSB1.cmake Thanks! but I need small modification, because on FreeBSD libusb1 has no '-1' suffix: http://people.freebsd.org/~makc/patches/FindLibUSB1.cmake Shall I open new request for extra-cmake-modules? Max - Max --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110271/#review32376 --- On May 11, 2013, 9:14 p.m., Max Brazhnikov wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110271/ --- (Updated May 11, 2013, 9:14 p.m.) Review request for kde-workspace. Description --- Use libusb-1 to query info about usb devices in kinfocenter. Remove *BSD specific code: it doesn't work on all supported FreeBSD versions. In principle it can be saved for NetBSD, but NetBSD could use libusb-1, thus drop it for simplification. Remove polling and use DeviceNotifier instead. Add FindLibUSB-1.cmake Diffs - cmake/modules/FindLibUSB-1.cmake PRE-CREATION kinfocenter/Modules/usbview/CMakeLists.txt 87bb256 kinfocenter/Modules/usbview/config-kcmusb.h.cmake PRE-CREATION kinfocenter/Modules/usbview/kcmusb.cpp c598467 kinfocenter/Modules/usbview/usbdevices.h 493caeb kinfocenter/Modules/usbview/usbdevices.cpp 9bd7033 Diff: http://git.reviewboard.kde.org/r/110271/diff/ Testing --- I've tested it only on FreeBSD. It would nice to test at least FindLibUSB-1.cmake on other OSes. Thanks, Max Brazhnikov
Re: Review Request 110271: libusb-1 support in kcmusb (kinfocenter)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110271/ --- (Updated May 11, 2013, 9:14 p.m.) Review request for kde-workspace. Changes --- Update FindLibUSB-1.cmake Description --- Use libusb-1 to query info about usb devices in kinfocenter. Remove *BSD specific code: it doesn't work on all supported FreeBSD versions. In principle it can be saved for NetBSD, but NetBSD could use libusb-1, thus drop it for simplification. Remove polling and use DeviceNotifier instead. Add FindLibUSB-1.cmake Diffs (updated) - cmake/modules/FindLibUSB-1.cmake PRE-CREATION kinfocenter/Modules/usbview/CMakeLists.txt 87bb256 kinfocenter/Modules/usbview/config-kcmusb.h.cmake PRE-CREATION kinfocenter/Modules/usbview/kcmusb.cpp c598467 kinfocenter/Modules/usbview/usbdevices.h 493caeb kinfocenter/Modules/usbview/usbdevices.cpp 9bd7033 Diff: http://git.reviewboard.kde.org/r/110271/diff/ Testing --- I've tested it only on FreeBSD. It would nice to test at least FindLibUSB-1.cmake on other OSes. Thanks, Max Brazhnikov
Re: Review Request 110271: libusb-1 support in kcmusb (kinfocenter)
Am 02.05.2013 15:49, schrieb Max Brazhnikov: Use libusb-1 to query info about usb devices in kinfocenter. Remove *BSD specific code: it doesn't work on all supported FreeBSD versions. In principle it can be saved for NetBSD, but NetBSD could use libusb-1, thus drop it for simplification. Remove polling and use DeviceNotifier instead. Add FindLibUSB-1.cmake First, the pkg-config stuff can be used on Windows. If the stuff isn't there it will simply do nothing, but it could. ok, I'll fix it. Then, find_library should never write it's stuff to a *_LIBRARIES variable as it will always only find one lib. The libraries variable should be composed from the libs found before and should not be a cached variable. ok. Finally, if the .pc provides a version you should use the newer interface of FPHSA that also allows version selection. Shouldn't cmake stuff work without pkg-config? Another problem is that FreeBSD has it's own implementation of libusb, which claims v1.0 api and there's no way to determine the version analogous to libusb.org version. Eike Max
Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter
Ok, here's a patch for GetInfo_ReadfromPipe only: http://people.freebsd.org/~makc/patches/read_from_pipe.diff Looks good to me (although I have not tested it). Small note: there is a contructor of QStringList taking a QString so if you have a list with only one item you don't need to construct an empty list and then add something using operator. Yes, I'm aware of it, but 'operator' looks more clear for me. Side-note: there is a comment talking about using /proc/pci if lspci is not found. Not that newer kernels do have a /proc/pci at all ;) I prefer not to make functional changes in (linux) code, which I can't test :) While you're at it you can then fix the type (devies - devices), too ;) I've removed those comments completely. Another solution for that problem ;) Eike Max
Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter
On Wed, 01 May 2013 14:44:39 +0200 Rolf Eike Beer wrote: 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#file139992l ine136 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. Hm, ok. But the 21 will not work, as that is shell syntax for redirects and I'm not sure if QProcess will unterstand that. What I basically had in mind was to pass a QStringList to the functions so that the arguments are already properly separated. Basically what the commenter here had in mind (info_fbsd.cpp): // TODO: GetInfo_ReadfromPipe should be improved so that we could pass the program name and its // arguments to it and remove most of the code below. So it would be nice if you could clean up that, too. Ok, here's a patch for GetInfo_ReadfromPipe only: http://people.freebsd.org/~makc/patches/read_from_pipe.diff While you're at it you can then fix the type (devies - devices), too ;) I've removed those comments completely. Max
Review Request 110271: libusb-1 support in kcmusb (kinfocenter)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110271/ --- Review request for kde-workspace. Description --- Use libusb-1 to query info about usb devices in kinfocenter. Remove *BSD specific code: it doesn't work on all supported FreeBSD versions. In principle it can be saved for NetBSD, but NetBSD could use libusb-1, thus drop it for simplification. Remove polling and use DeviceNotifier instead. Add FindLibUSB-1.cmake Diffs - cmake/modules/FindLibUSB-1.cmake PRE-CREATION kinfocenter/Modules/usbview/CMakeLists.txt 87bb256 kinfocenter/Modules/usbview/config-kcmusb.h.cmake PRE-CREATION kinfocenter/Modules/usbview/kcmusb.cpp c598467 kinfocenter/Modules/usbview/usbdevices.h 493caeb kinfocenter/Modules/usbview/usbdevices.cpp 9bd7033 Diff: http://git.reviewboard.kde.org/r/110271/diff/ Testing --- I've tested it only on FreeBSD. It would nice to test at least FindLibUSB-1.cmake on other OSes. Thanks, Max Brazhnikov
Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter
--- 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 (updated) - 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
Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter
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
Review Request 110090: Clean up kickoff from stale bits
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110090/ --- Review request for kde-workspace. Description --- kickoff links to strigiqtdbusclient, but Strigi is not used since svn r1018482. Diffs - plasma/desktop/applets/kickoff/CMakeLists.txt e9e2888 plasma/desktop/applets/kickoff/core/config-kickoff-applets.h.cmake cecf380 Diff: http://git.reviewboard.kde.org/r/110090/diff/ Testing --- Thanks, Max Brazhnikov
Review Request 110091: clean up and update FreeBSD support for kinfocenter
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110091/ --- Review request for kde-workspace. Description --- Add FindLibdevinfo.cmake Clean up info_fbsd.cpp and utilize PCIUtils Diffs - cmake/modules/FindLibdevinfo.cmake e69de29 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
