D6992: Fix build on Linux without X11
This revision was automatically updated to reflect the committed changes. Closed by commit R285:02d555f9db3e: Fix build on Linux without X11 (authored by vkrause). REPOSITORY R285 KCrash CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6992?vs=24251&id=24270 REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES src/kcrash.cpp To: vkrause, #frameworks, bshah, graesslin, dfaure Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
dfaure accepted this revision. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin, dfaure Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause updated this revision to Diff 24251. vkrause added a comment. follow kinit changes REPOSITORY R285 KCrash CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6992?vs=24227&id=24251 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES src/kcrash.cpp To: vkrause, #frameworks, bshah, graesslin, dfaure Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause updated this revision to Diff 24227. vkrause added a comment. Remove dead code. REPOSITORY R285 KCrash CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6992?vs=24225&id=24227 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES src/kcrash.cpp To: vkrause, #frameworks, bshah, graesslin, dfaure Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause added inline comments. INLINE COMMENTS > dfaure wrote in kcrash.cpp:271 > I wonder why this isn't removed then, it can't happen ;-) Indeed, I'll clean it up here and in kinit. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin, dfaure Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
dfaure accepted this revision. dfaure added a comment. Looks much better indeed, thanks. INLINE COMMENTS > kcrash.cpp:271 > return "MAC_DISPLAY"; > #elif defined(Q_OS_WIN) > return "WIN_DISPLAY"; I wonder why this isn't removed then, it can't happen ;-) REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin, dfaure Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause updated this revision to Diff 24225. vkrause added a comment. Re-sync with KInit, and thus solve the non-X11 compile issue in the same way as it's handled there already. REPOSITORY R285 KCrash CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6992?vs=24216&id=24225 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES src/kcrash.cpp To: vkrause, #frameworks, bshah, graesslin Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
dfaure added a comment. 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. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause added a comment.
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.
REPOSITORY
R285 KCrash
REVISION DETAIL
https://phabricator.kde.org/D6992
To: vkrause, #frameworks, bshah, graesslin
Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
dfaure added a comment.
so this is going to call getenv("") ? Or is this code path never called?
REPOSITORY
R285 KCrash
REVISION DETAIL
https://phabricator.kde.org/D6992
To: vkrause, #frameworks, bshah, graesslin
Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause updated this revision to Diff 24216. vkrause added a comment. Reduced to a minimal compile fix for now. REPOSITORY R285 KCrash CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6992?vs=17388&id=24216 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES src/kcrash.cpp To: vkrause, #frameworks, bshah, graesslin Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause added a task: T6924: Fix KF5 compilation without X11. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
dfaure added a comment. 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. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: dfaure, graesslin, bshah
D6992: Fix build on Linux without X11
vkrause added a comment. 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. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: graesslin, bshah
D6992: Fix build on Linux without X11
graesslin added a comment. But then the root problem of not having an ifdef branch as fallback is also not fixed. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: graesslin, bshah
D6992: Fix build on Linux without X11
vkrause added a comment. 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 :) REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: graesslin, bshah
D6992: Fix build on Linux without X11
graesslin requested changes to this revision. graesslin added a comment. This revision now requires changes to proceed. 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. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah, graesslin Cc: graesslin, bshah
D6992: Fix build on Linux without X11
vkrause updated this revision to Diff 17388. REPOSITORY R285 KCrash CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6992?vs=17387&id=17388 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES CMakeLists.txt src/config-kcrash.h.cmake src/kcrash.cpp To: vkrause, #frameworks, bshah Cc: bshah
D6992: Fix build on Linux without X11
bshah added a comment. Also, you will need to make similar change on kinit.cpp in kinit. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah Cc: bshah
D6992: Fix build on Linux without X11
bshah requested changes to this revision. bshah added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kcrash.cpp:272 > return "WIN_DISPLAY"; > +#else > +return "WAYLAND_DISPLAY"; That sounds wrong.. possibly you have to check if HAVE_WAYLAND is true.. REPOSITORY R285 KCrash REVISION DETAIL https://phabricator.kde.org/D6992 To: vkrause, #frameworks, bshah Cc: bshah
D6992: Fix build on Linux without X11
vkrause created this revision. Restricted Application added a project: Frameworks. REPOSITORY R285 KCrash BRANCH master REVISION DETAIL https://phabricator.kde.org/D6992 AFFECTED FILES src/kcrash.cpp To: vkrause, #frameworks
