Re: [Maria-developers] b4cdf20dd15: MDEV-17591 Create MariaDB named commands/symlinks

Hi, Rasmus! On Jun 14, Rasmus Johansson wrote:

revision-id: b4cdf20dd15 (mariadb-10.4.5-44-gb4cdf20dd15) parent(s): 8e3a4be45c5 author: Rasmus Johansson committer: Rasmus Johansson timestamp: 2019-06-14 14:55:20 +0000 message:

MDEV-17591 Create MariaDB named commands/symlinks

diff --git a/cmake/symlinks.cmake b/cmake/symlinks.cmake new file mode 100644 index 00000000000..32cf24e3e3b --- /dev/null +++ b/cmake/symlinks.cmake @@ -0,0 +1,76 @@ +# MariaDB names for executables +list(APPEND MARIADB_SYMLINK_NAMES "mysql" "mariadb") +list(APPEND MARIADB_SYMLINK_NAMES "mysqlaccess" "mariadb-access") +list(APPEND MARIADB_SYMLINK_NAMES "mysqladmin" "mariadb-admin") +list(APPEND MARIADB_SYMLINK_NAMES "mariabackup" "mariadb-backup") ... +# Add MariaDB symlinks +macro(CREATE_MARIADB_SYMLINK src comp) + # Find the MariaDB name for executable + list(FIND MARIADB_SYMLINK_NAMES ${src} _index) + + if (${_index} GREATER -1) + MATH(EXPR _index "${_index}+1") + list(GET MARIADB_SYMLINK_NAMES ${_index} _name) + set(mariadbname ${_name}) + endif() + + if (mariadbname) + set(dest ${mariadbname}) + set(symlink_install_dir ${INSTALL_BINDIR}) + + # adjust install location if needed + if(${dest} MATCHES "mariadb-install-db") + set(symlink_install_dir ${INSTALL_SCRIPTDIR}) + endif()

I still think it's very fragile and difficult to maintain. Why not to pass the destination as a third argument to CREATE_MARIADB_SYMLINK? As far as I can see, everywhere where you use CREATE_MARIADB_SYMLINK the destination is available. The rest is okay. I'd still suggest to simplify by splitting the list in two, like macro(REGISTER_SYMLINK from to) list(APPEND MARIADB_SYMLINK_FROMS from) list(APPEND MARIADB_SYMLINK_TOS to) endmacro() REGISTER_SYMLINK(mysql mariadb) REGISTER_SYMLINK(mysqladmin mariadb-access) ...etc... Then you won't need to do math on indexes, you simply search in MARIADB_SYMLINK_FROMS and get the same element in MARIADB_SYMLINK_TOS. Also you will need to search only in MARIADB_SYMLINK_FROMS, not like now, when you search in the complete list, risking to get spurious matches on mariadb* names. Same in manpages, you'll only iterate over MARIADB_SYMLINK_FROMS list.

+ + CREATE_MARIADB_SYMLINK_IN_DIR(${src} ${dest} ${symlink_install_dir} ${comp}) + endif() +endmacro(CREATE_MARIADB_SYMLINK)

Regards, Sergei Chief Architect MariaDB and security@mariadb.org