maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Rasmus!

On Jun 14, Rasmus Johansson wrote:
> revision-id: b4cdf20dd15 (mariadb-10.4.5-44-gb4cdf20dd15)
> parent(s): 8e3a4be45c5
> author: Rasmus Johansson <rasmus@xxxxxxxxxxx>
> committer: Rasmus Johansson <rasmus@xxxxxxxxxxx>
> 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@xxxxxxxxxxx