Details
- Reviewers
bshah graesslin dfaure - Group Reviewers
Frameworks - Maniphest Tasks
- T6924: Fix KF5 compilation without X11
- Commits
- R285:02d555f9db3e: Fix build on Linux without X11
Diff Detail
- Repository
- R285 KCrash
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
src/kcrash.cpp | ||
---|---|---|
272 | That sounds wrong.. possibly you have to check if HAVE_WAYLAND is true.. |
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.
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.
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.
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.
Re-sync with KInit, and thus solve the non-X11 compile issue in the same way as it's handled there already.
Looks much better indeed, thanks.
src/kcrash.cpp | ||
---|---|---|
270 | I wonder why this isn't removed then, it can't happen ;-) |
src/kcrash.cpp | ||
---|---|---|
270 | Indeed, I'll clean it up here and in kinit. |