Commit 29ea3d65 authored by Andrey Butirsky's avatar Andrey Butirsky
Browse files

fix: components dir content is duplicated for sddm

preserve symlink on install
parent 69f5c3a0
......@@ -178,8 +178,11 @@ if (INSTALL_SDDM_THEME)
PATTERN "theme.conf.cmake" EXCLUDE)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/sddm-theme/theme.conf DESTINATION ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze)
install(DIRECTORY lookandfeel/contents/components DESTINATION ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze PATTERN "README.txt" EXCLUDE)
install(CODE "if (NOT EXISTS ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze/components)
file(RELATIVE_PATH relpath ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze ${KDE_INSTALL_FULL_DATADIR}/plasma/look-and-feel/org.kde.breeze.desktop/contents/components)
execute_process(COMMAND ${CMAKE_COMMAND} -E create_symlink \${relpath} ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze/components)
  • This is causing sandbox violation for distribution packaging builds.

  • I'm not quite sure I understand the problem correctly, but do you have any solution in mind other than reverting it?
    Could we change path in QML import statements to overcome it?

  • @butirsky I'm not sure if this is correct but here's a possible fix. It stops the sandbox errors in Gentoo:

    diff --git a/CMakeLists.txt b/CMakeLists.txt
    index 0fc13fd83..a1f057763 100644
    --- a/CMakeLists.txt
    +++ b/CMakeLists.txt
    @@ -178,9 +178,10 @@ if (INSTALL_SDDM_THEME)
         install(DIRECTORY sddm-theme/ DESTINATION ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze PATTERN "README.txt" EXCLUDE PATTERN "components" EXCLUDE PATTERN "dummydata" EXCLUDE
         PATTERN "theme.conf.cmake" EXCLUDE)
         install(FILES ${CMAKE_CURRENT_BINARY_DIR}/sddm-theme/theme.conf DESTINATION ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze)
    -    install(CODE "if (NOT EXISTS ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze/components)
    +    install(CODE "if (NOT EXISTS \$ENV{DESTDIR}/${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze/components)
                         file(RELATIVE_PATH relpath ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze ${KDE_INSTALL_FULL_DATADIR}/plasma/look-and-feel/org.kde.breeze.desktop/contents/components)
    -                    execute_process(COMMAND ${CMAKE_COMMAND} -E create_symlink \${relpath} ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze/components)
    +                    execute_process(COMMAND ${CMAKE_COMMAND} -E create_symlink
    +						\${relpath} \$ENV{DESTDIR}/${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze/components)
    Edited by Aidan Harris
  • Thanks for the tip @aidanharris.
    I don't see that DESTDIR anywhere else, however. Do you have a clue why now it's needed in these lines?

  • @butirsky When distros build KDE their build system installs into a temporary directory set by DESTDIR ( Without this you are trying to write directly to the root filesystem which might fail (either due to a sandbox like Gentoo which blocks writes to the root filesystem, or because you don't have permission to write).

    The diff above makes it so that if DESTDIR=/var/tmp/portage/kde-plasma/plasma-workspace/image the symlink will be created in /var/tmp/portage/kde-plasma/image/usr/share/sddm/themes/breeze/components instead of writing directly to the root filesystem. When the distro packages up this directory it will still point to the correct place (../../../plasma/look-and-feel/org.kde.breeze.desktop/contents/components).

  • Thanks, just wonder why it's not needed in the other places in the file..
    Maybe install(DIRECTORY) takes it into account, and install(CODE) doesn't?

  • I assume so.

    What about using file(CREATE_LINK ... SYMBOLIC) with install(FILES ...) instead?

  • @fvogt the variant with file(CREATE_LINK ... SYMBOLIC) was my first attempt which broke the build: !393 (merged)

  • That looks like it was because you used file to create the target symlink during configuration already instead of leaving that to install.

  • Yes. So what do you mean by "using file(CREATE_LINK ... SYMBOLIC) with install(FILES ...)" then?

  • Something like this (untested):

    file(CREATE_LINK ${KDE_INSTALL_FULL_DATADIR}/plasma/look-and-feel/org.kde.breeze.desktop/contents/components ${CMAKE_CURRENT_BINARY_DIR}/sddm-theme/components SYMBOLIC)
    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/sddm-theme/components DESTINATION ${KDE_INSTALL_FULL_DATADIR}/sddm/themes/breeze)
  • Ah, didn't think I could use ${CMAKE_CURRENT_BINARY_DIR} as temporary link storage. Thanks!
    Do you guys need it really soon? I'm not sure I have time just right now.
    So MR is welcome, or for me it would be more comfortable if it could wait for tomorrow.

  • In that case I'd vote for a temporary revert.

    I'm also wondering about the if (NOT EXISTS part, doesn't that mean that if the target already exists (as wrong symlink or plain dir, it'll never get fixed with this code?

  • Yeah, feel free to revert it. I'll provide third attempt then ;)
    if (NOT EXISTS part is not necessary but prevents unconditional link overriding every build.
    It works right for broken links, and the plain dir case assumed as unlikely..

  • BTW, it revealed we don't have this distro-packaging stage covered in CI, which is not good probably.

  • Revert pushed: 82400960

    Just mention @asturmlechner and me and we'll do a test run.

    BTW, it didn't fail package build here (make install had exit code 0), apparently cmake silently ignored the failure.

Please register or sign in to reply
Supports Markdown
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