> On June 16, 2014, 6:57 a.m., Martin Gräßlin wrote:
> > looks good to me, +1. Please add Hugo Pereira Da Costa to the Review
> > Request, though.
> >
> > The review request made me wonder whether we still need to find XLib in
> > Oxygen, though. The parts shown only use XCB, so maybe we could just go for
> > finding only XCB?
>
> Hugo Pereira Da Costa wrote:
> @Martin,
> yes you are probably right. X11 should not be necessary any more.
> I'll double check and commit. (have other stuff pending).
>
@Martin, I think you are correct.
However, in the top CMakeLists I see:
find_package(X11)
if(X11_FOUND)
find_package(XCB REQUIRED COMPONENTS XCB)
set_package_properties(XCB PROPERTIES TYPE REQUIRED)
find_package(Qt5 REQUIRED CONFIG COMPONENTS X11Extras)
else()
set(X11_FOUND 0)
endif()
How would I deal with that ? (the X11_FOUND part), if I don't look for X11 in
the first place ?
Is there a XCB_FOUND set by find_packgage ?
Should I make XCB optional (as X11 is) ? and then replace all instances of
X11_FOUND by XCB_FOUND ?
If this is the right way, I'll do and commit
Thanks in advance
- Hugo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118763/#review60162
-----------------------------------------------------------
On Aug. 22, 2014, 3:31 p.m., Bernd Steinhauser wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118763/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2014, 3:31 p.m.)
>
>
> Review request for kde-workspace, Plasma and Hugo Pereira Da Costa.
>
>
> Repository: oxygen
>
>
> Description
> -------
>
> No idea if kde-workspace is still the right group, if not, please change.
>
> find_package(XCB) is called without specifying the required components. This
> leads to linking to unused dependencies in case they are installed.
>
> Since XCB is searched for in the top level cmake file in the repository,
> there is no need to search for it again. The component required there (only
> base XCB) is sufficient.
> Although, this should be sufficient to fix the deps problem, it makes sense
> to link to XCB::XCB instead of ${XCB_LIBRARIES}, since the former is what is
> actually needed.
>
>
> Diffs
> -----
>
> kstyle/CMakeLists.txt 165b62a
> liboxygen/CMakeLists.txt 0d1dd94
>
> Diff: https://git.reviewboard.kde.org/r/118763/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bernd Steinhauser
>
>
