sitter requested changes to this revision.
sitter added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> KDEInstallDirs.cmake:699
> +if(INSTALL_PREFIX_SCRIPT)
> +    file(WRITE ${CMAKE_BINARY_DIR}/prefix.sh "
> +export XDG_DATA_DIRS=${KDE_INSTALL_FULL_DATADIR}:$XDG_DATA_DIRS

>From a style perspective, I'd suggest having the prefix.sh live somewhere in 
>the installed ECM tree and get copied, rather than maintained as a glorified 
>heredoc in the cmake code. That's just a suggestion though.

> KDEInstallDirs.cmake:700
> +    file(WRITE ${CMAKE_BINARY_DIR}/prefix.sh "
> +export XDG_DATA_DIRS=${KDE_INSTALL_FULL_DATADIR}:$XDG_DATA_DIRS
> +export XDG_CONFIG_DIRS=${KDE_INSTALL_FULL_CONFDIR}:$XDG_CONFIG_DIRS

This is not correct, the XDG_ vars are not necessarily set, so all code that 
sets them ought to ensure their default values are appended if necessary.

> If $XDG_DATA_DIRS is either not set or empty, a value equal to 
> /usr/local/share/:/usr/share/ should be used.

i.e.

  export 
XDG_DATA_DIRS=${KDE_INSTALL_FULL_DATADIR}:${XDG_DATA_DIRS:-/usr/local/share:/usr/share}

> KDEInstallDirs.cmake:701
> +export XDG_DATA_DIRS=${KDE_INSTALL_FULL_DATADIR}:$XDG_DATA_DIRS
> +export XDG_CONFIG_DIRS=${KDE_INSTALL_FULL_CONFDIR}:$XDG_CONFIG_DIRS
> +export PATH=${KDE_INSTALL_FULL_BINDIR}:$PATH

Same as for XDG_DATA_DIRS

> If $XDG_CONFIG_DIRS is either not set or empty, a value equal to /etc/xdg 
> should be used."

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D9299

To: apol, #frameworks, sitter
Cc: sitter, cgiboudeaux, #build_system

Reply via email to