Hi Serg,

On Thu, Jun 13, 2019 at 11:18 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Rasmus!

On Jun 13, Rasmus Johansson wrote:
> revision-id: e35fb718eae (mariadb-10.4.5-44-ge35fb718eae)
> parent(s): 8e3a4be45c5
> author: Rasmus Johansson <razze@iki.fi>
> committer: Rasmus Johansson <razze@iki.fi>

Is that an address you want to have in git history?
You already have two, just making sure the third one is intentional.

I changed this for the latest commit. Thanks for pointing it out.
 
> timestamp: 2019-06-13 13:20:40 +0000
> message:
>
> MDEV-17591 Create MariaDB named commands/symlinks
>
> diff --git a/cmake/install_macros.cmake b/cmake/install_macros.cmake
> index f28720e97bc..e49f296ed75 100644
> --- a/cmake/install_macros.cmake
> +++ b/cmake/install_macros.cmake
> @@ -111,8 +111,10 @@ FUNCTION(INSTALL_SCRIPT)
>    ENDIF()

>    INSTALL(PROGRAMS ${script} DESTINATION ${ARG_DESTINATION} ${COMP})
> +  string(REPLACE "${CMAKE_BINARY_DIR}/scripts/" "" dest ${script})

better not to assume that the script is in ${CMAKE_BINARY_DIR}/scripts/
I'd use get_filename_component() here. Like

 get_filename_component(dest "${scripts}" NAME)

Changed accordingly.
 
> +# Add MariaDB symlinks
> +macro(CREATE_MARIADB_SYMLINK src)

do you still skip Windows, as in the previous commit? I couldn't find it
in this one.

Yes, Windows will be in a seperate commit.
 
> +
> +    add_custom_target(
> +      symlink_${dest} ALL
> +      DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/${dest}
> +    )
> +
> +    add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/${dest} POST_BUILD
> +      COMMAND ${CMAKE_COMMAND} -E create_symlink ${src} ${dest}
> +      COMMENT "mklink ${src} -> ${dest}")
> +
> +    set(symlink_install_dir ${INSTALL_BINDIR})
> +    # adjust install location if needed
> +    if(${dest} MATCHES "mariadb-install-db")
> +      set(symlink_install_dir ${INSTALL_SCRIPTDIR})
> +    endif()

this is very fragile. better to take the path from the ${src}
also with get_filename_component().

In this case it doesn't really make sense to use get_filename_component because here $dest is already only the filename, no path.
 
> +usr/share/man/man1/mysql_find_rows.1.gz usr/share/man/man1/mariadb-find-rows.1.gz
> +usr/share/man/man1/mysql_fix_extensions.1.gz usr/share/man/man1/mariadb-fix-extensions.1.gz

manpages aren't symlinked in rpms or bintars.
and symlinks depends only on MariaDB-client, while
for example mysql_install_db is in the server package.

I've now added the manpage symlinks to bintar and rpms also.
 
may be just put symlinks into the same package as symlink targets?
just like with debs?

This also done now for rpm.

The new commit is here: https://github.com/MariaDB/server/commit/270c73b

Rasmus