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=24270
REVISION DETAIL
dfaure accepted this revision.
REPOSITORY
R285 KCrash
REVISION DETAIL
https://phabricator.kde.org/D6992
To: vkrause, #frameworks, bshah, graesslin, dfaure
Cc: dfaure, graesslin, bshah
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=24251
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6992
AFFECTED FILES
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=24227
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6992
AFFECTED FILES
src/kcrash.cpp
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,
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
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=24225
BRANCH
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,
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
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
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=24216
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6992
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
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,
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
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
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
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
vkrause updated this revision to Diff 17388.
REPOSITORY
R285 KCrash
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6992?vs=17387=17388
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D6992
AFFECTED FILES
CMakeLists.txt
src/config-kcrash.h.cmake
src/kcrash.cpp
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
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..
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
21 matches
Mail list logo
