Fix build on Linux without X11
ClosedPublic

Authored by vkrause on Jul 30 2017, 1:03 PM.

Diff Detail

Repository
R285 KCrash
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
vkrause created this revision.Jul 30 2017, 1:03 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 30 2017, 1:03 PM
bshah requested changes to this revision.Jul 30 2017, 1:05 PM
bshah added a subscriber: bshah.
bshah added inline comments.
src/kcrash.cpp
272

That sounds wrong.. possibly you have to check if HAVE_WAYLAND is true..

This revision now requires changes to proceed.Jul 30 2017, 1:05 PM
bshah added a comment.Jul 30 2017, 1:05 PM

Also, you will need to make similar change on kinit.cpp in kinit.

vkrause updated this revision to Diff 17388.Jul 30 2017, 1:18 PM
vkrause edited edge metadata.
graesslin requested changes to this revision.Jul 30 2017, 2:50 PM
graesslin added a subscriber: graesslin.

Sorry to say, but the approach is wrong in general. The problem is that the platform is runtime selected and not compile time. This patch fixes a very special case which does not exist in reality: distros build with both X11 and Wayland.

This revision now requires changes to proceed.Jul 30 2017, 2:50 PM

I agree on this needing runtime rather than compile-time selection code. I disagree on this not happening in reality though, the KF5 Yocto recipes assume there is no X11, which is why I ran into this in the first place :)

But then the root problem of not having an ifdef branch as fallback is also not fixed.

Ok, now I've hit the same problem in kinit, but unlike the comment in the code here suggests it's not actually identical... So, how should this work in an ideal scenario?

kinit/src/wrapper.cpp would use an empty display string if neither X11 nor XCB are found at compile time. Is that good enough, or do we want something like use DISPLAY if present, otherwise use WAYLAND_DISPLAY if present, otherwise error out on non-macos/non-windows? I can't seem to find any reference about MAC_DISPLAY or WIN_DISPLAY, so no idea what to do about those cases.

dfaure added a subscriber: dfaure.Aug 8 2017, 7:34 AM

I think MAC_DISPLAY and WIN_DISPLAY just don't exist, they are historical relics of the first attempts at portability in this code. But I could be wrong.

vkrause updated this revision to Diff 24216.Dec 21 2017, 12:32 PM

Reduced to a minimal compile fix for now.

so this is going to call getenv("") ? Or is this code path never called?

It's going to call getenv("") and then continue with the empty display variable case for non-X11 below. At least that's the assumption, an entirely X11-free setup is still somewhat theoretical at this point :)
My goal for now is to make the KF5 Yocto recipes useful in a non-X11/pure Wayland setup, kcrash matters there due to being a wide-spread (indirect) dependency.

Hmm, OK. Sounds like the whole thing could easily be refactored to not call qgetenv at all on non-X11... (i.e. including on OSX/Windows). That would be much less confusing.

vkrause updated this revision to Diff 24225.Dec 21 2017, 1:56 PM

Re-sync with KInit, and thus solve the non-X11 compile issue in the same way as it's handled there already.

dfaure accepted this revision.Dec 21 2017, 2:00 PM

Looks much better indeed, thanks.

src/kcrash.cpp
270

I wonder why this isn't removed then, it can't happen ;-)

vkrause added inline comments.Dec 21 2017, 2:05 PM
src/kcrash.cpp
270

Indeed, I'll clean it up here and in kinit.

vkrause updated this revision to Diff 24227.Dec 21 2017, 2:08 PM

Remove dead code.

vkrause updated this revision to Diff 24251.Dec 21 2017, 4:12 PM

follow kinit changes

dfaure accepted this revision.Dec 21 2017, 8:34 PM
This revision was automatically updated to reflect the committed changes.