Commit a6b28911 authored by Gustavo Carneiro's avatar Gustavo Carneiro

Revert "login-sessions: Set the correct filename depending on default session type"

This reverts commit b93a0639.
parent 8b4cf5a1
......@@ -193,7 +193,6 @@ add_definitions(-DQT_NO_URL_CAST_FROM_STRING)
query_qmake(QtBinariesDir QT_INSTALL_BINS)
option(PLASMA_SYSTEMD_BOOT "Use systemd units for startup of plasma (WIP)" FALSE)
option(PLASMA_WAYLAND_DEFAULT_SESSION "Use Wayland session by default for Plasma" FALSE)
add_subdirectory(doc)
add_subdirectory(libkworkspace)
......
## For Plasma end users
if(PLASMA_WAYLAND_DEFAULT_SESSION)
set(PLASMA_X11_DESKTOP_FILENAME plasmax11.desktop CACHE INTERNAL)
set(PLASMA_WAYLAND_DESKTOP_FILENAME plasma.desktop CACHE INTERNAL)
else()
set(PLASMA_X11_DESKTOP_FILENAME plasma.desktop CACHE INTERNAL)
set(PLASMA_WAYLAND_DESKTOP_FILENAME plasmawayland.desktop CACHE INTERNAL)
endif()
configure_file(plasmax11.desktop.cmake ${CMAKE_CURRENT_BINARY_DIR}/${PLASMA_X11_DESKTOP_FILENAME})
configure_file(plasmax11.desktop.cmake ${CMAKE_CURRENT_BINARY_DIR}/plasmax11.desktop)
install(FILES
${CMAKE_CURRENT_BINARY_DIR}/${PLASMA_X11_DESKTOP_FILENAME}
${CMAKE_CURRENT_BINARY_DIR}/plasmax11.desktop
DESTINATION ${KDE_INSTALL_DATADIR}/xsessions
)
configure_file(plasmawayland.desktop.cmake ${CMAKE_CURRENT_BINARY_DIR}/${PLASMA_WAYLAND_DESKTOP_FILENAME})
configure_file(plasmawayland.desktop.cmake ${CMAKE_CURRENT_BINARY_DIR}/plasmawayland.desktop)
install(FILES
${CMAKE_CURRENT_BINARY_DIR}/${PLASMA_WAYLAND_DESKTOP_FILENAME}
${CMAKE_CURRENT_BINARY_DIR}/plasmawayland.desktop
DESTINATION ${KDE_INSTALL_DATADIR}/wayland-sessions
)
......
  • Pardon me but where was this discussed? Or reviewed?

  • While there's a lack of communication on the commit, this is actually the correct thing to do:

    $ git revert a6b28911869f00258
    [check_build_failure 59eca83c8] Revert "Revert "login-sessions: Set the correct filename depending on default session type""
     2 files changed, 13 insertions(+), 4 deletions(-)
    
    $ cd ../../build/plasma-workspace
    $ make
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    -- Found KF5: success (found suitable version "5.78.0", minimum required is "5.77") found components: Plasma DocTools Runner Notifications NotifyConfig Su NewStuff Wallet IdleTime Declarative I18n KCMUtils TextWidgets KDELibs4Support Crash GlobalAccel DBusAddons Wayland CoreAddons People ActivitiesStats Activities KIO Prison 
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    -- Found KF5: success (found version "5.78.0") found components: PlasmaQuick 
    Installing in /home/tcanabrava/KDE/install. Run /data/Projects/KDE/build/plasma-workspace/prefix.sh to set the environment for plasma-workspace.
    -- Found KF5: success (found version "5.78.0") found components: Package 
    -- Warning: Property DESCRIPTION for package Fontconfig already set to "Fontconfig is a library for configuring and customizing font access", overriding it with "Font access configuration library"
    -- Warning: Property URL already set to "https://www.fontconfig.org/", overriding it with "https://www.freedesktop.org/wiki/Software/fontconfig"
    -- Found Wayland: /usr/lib/libwayland-client.so (found version "1.18.0") found components: Client 
    -- XCB: IMAGE requires XCB;SHM
    -- Found XCB: /usr/lib/libxcb.so;/usr/lib/libxcb-shm.so;/usr/lib/libxcb-image.so;/usr/lib/libxcb-randr.so (found version "1.14") found components: XCB RANDR IMAGE 
    CMake Error at login-sessions/CMakeLists.txt:7 (set):
      set given invalid arguments for CACHE mode.
    
    
    CMake Error at login-sessions/CMakeLists.txt:8 (set):
      set given invalid arguments for CACHE mode.
    
    
    CMake Error at login-sessions/CMakeLists.txt:12 (install):
      install FILES given directory
      "/data/Projects/KDE/build/plasma-workspace/login-sessions/" to install.
    
    
    CMake Error at login-sessions/CMakeLists.txt:18 (install):
      install FILES given directory
      "/data/Projects/KDE/build/plasma-workspace/login-sessions/" to install.
    
    ... snip ...
    
    -- Configuring incomplete, errors occurred!
    See also "/data/Projects/KDE/build/plasma-workspace/CMakeFiles/CMakeOutput.log".
    See also "/data/Projects/KDE/build/plasma-workspace/CMakeFiles/CMakeError.log".
    make: *** [Makefile:5619: cmake_check_build_system] Error 1
    
    $

    So the real errors are: 1 - broken commit on master on the original commit 2 - revert without explanation of the reason

  • Where the problem with the original commit -- which wasn't caught in review and hasn't systematically broken builds, which is weird -- is this bit:

    set(PLASMA_X11_DESKTOP_FILENAME plasmax11.desktop CACHE INTERNAL)

    where the CMake documentation says that a docstring is mandatory after CACHE:

     set(<variable> <value>... CACHE <type> <docstring> [FORCE])
    
    The ``<docstring>`` must be specified as a line of text providing
    a quick summary of the option for presentation to ``cmake-gui(1)``
    users.
  • Hi, I apologize for this immediate revert, I worried about making things work correctly and ended up forgetting to explain the reason for the revert. Tomaz already had a conversation with me and guided me on how to proceed correctly in these cases, so I hope it didn't cause any problems.

  • With this revert, 71dfc770 is practically a noop with the only visible change being breakage due to plasma.desktop vanishing completely. IMO 71dfc770 should be reverted as well.

  • Or, you know, I can just submit a fixed version of the commit again?

  • That's obviously fine as well.

  • Neal Gompa @ngompa

    mentioned in merge request !532 (merged)

    ·

    mentioned in merge request !532 (merged)

    Toggle commit list
  • MR with fixed commit: !532 (merged)

  • Toggle commit list
  • mentioned in commit c0415515

    Toggle commit list
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment