Hi, Daniel,

This is a review of `git diff 12786f0e779f 5f76981eb649`, that is, for
commits:

5f76981eb64 rpm: packages that dont match component name
25eaa0aebfa MDEV-30205: [Cmtr: suite look in mariadb-test path
0981201f027 mtr - wsrep use mariadb executables
eba0b35035a mtr: maradb names for *embedded and mariadb-client-test
974ca88f0fe deb: fix autotest - exclude for MDEV-30205
b19d75408bb Deb: Breaks/Replaces/COnflicts
c60f0341950 MDEV-30203 deb breaks/replaces on compat packages
f984f49fba7 MDEV-30203 - deb fix piuparts
310d025ff0d MDEV-30205: fix deb autopkgtest
89f6cf47ee2 rpm: fix packaging of compat/non-compat scripts links
494ed76d15a deb: mysqlhotcopy to mariadb-client-compat
d03075f3a1f deb: server-core mysql* -> server-compat
9453812193d wsrep_sst_mariabackup to use mariadb-backup (and message accordingly)
a4f843a7a5a Fix uncollected symlink
eb9a469bf9a Add package recommended dependencies
c135be62274 Remove duplicate entry
84c7cd29c61 Fix Debian packaging for mariabackup
5dbc7ae8bb4 MDEV-30203: compat packages - RPM  remove mariabackup link
eb4fa5d750f MDEV-30203: debian - make compat packages
d4be0b9be73 MDEV-30203: Create mysql symlink packages (RPM) - scripts
441879fd493 MDEV-30203: Create mysql symlink packages (RPM)

On Dec 19, Daniel Black wrote:

> diff --git a/cmake/cpack_rpm.cmake b/cmake/cpack_rpm.cmake
> index 339363b2169..7b48fd64f4c 100644
> --- a/cmake/cpack_rpm.cmake
> +++ b/cmake/cpack_rpm.cmake
> @@ -85,6 +85,35 @@ SET(CPACK_RPM_server_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}")
>  SET(CPACK_RPM_test_PACKAGE_SUMMARY "MariaDB database regression test suite")
>  SET(CPACK_RPM_test_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}")
> 
> +# "set/append array" - append a set of strings, separated by a space
> +MACRO(SETA var)
> +  FOREACH(v ${ARGN})
> +    SET(${var} "${${var}} ${v}")
> +  ENDFOREACH()
> +ENDMACRO(SETA)
> +
> +FOREACH(SYM_COMPONENT Server Client Test)
> +  SET(SYM ${SYM_COMPONENT}_symlinks)
> +  string(TOLOWER ${SYM} SYM)

* why all cmake commands are uppercase, but `string` is lowercase?
* SET is redundant, you can do

  STRING(TOLOWER ${SYM_COMPONENT}_symlinks SYM)

* may be, better to use ${SYM_COMPONENT}-symlinks ?
  Like in MariaDB-server-symlinks ?

> +  SET(SYMCOMP ${SYM_COMPONENT}Symlinks)
> +  string(TOUPPER ${SYMCOMP} SYMCOMP_UPPER)
> +  SET(CPACK_COMPONENT_${SYMCOMP_UPPER}_GROUP "${SYM}")
> +  SET(CPACK_COMPONENTS_ALL "${CPACK_COMPONENTS_ALL}" "${SYMCOMP}")
> +  SET(CPACK_RPM_${SYM}_PACKAGE_SUMMARY "MySQL compatible symlinks for MariaDB database ${SYM_COMPONENT} binaries/scripts")
> +  SET(CPACK_RPM_${SYM}_PACKAGE_DESCRIPTION "${CPACK_RPM_PACKAGE_DESCRIPTION}")
> +  SET(CPACK_RPM_${SYM}_PACKAGE_ARCHITECTURE "noarch")
> +  SETA(CPACK_RPM_${SYM_COMPONENT}_PACKAGE_RECOMMENDS "MariaDB-${SYM}")

You forgot to say that symlink package DEPENDS on the non-symlink package

> +ENDFOREACH()
> +
> +# TODO change to 11.0 on rebase
> +SETA(CPACK_RPM_client_symlinks_PACKAGE_CONFLICTS
> +  "MariaDB-client < 10.11.2"
> +  "MariaDB-server < 10.11.2")
> +SETA(CPACK_RPM_server_symlinks_PACKAGE_CONFLICTS
> +  "MariaDB-server < 10.11.2")
> +SETA(CPACK_RPM_test_symlinks_PACKAGE_CONFLICTS
> +  "MariaDB-test < 10.11.2")
> +
>  # libmariadb3
>  SET(CPACK_RPM_shared_PACKAGE_SUMMARY "LGPL MariaDB database client library")
>  SET(CPACK_RPM_shared_PACKAGE_DESCRIPTION "This is LGPL MariaDB client library that can be used to connect to MySQL
> diff --git a/cmake/symlinks.cmake b/cmake/symlinks.cmake
> index 3f3b4e4a9b5..a4eb6c2b42c 100644
> --- a/cmake/symlinks.cmake
> +++ b/cmake/symlinks.cmake
> @@ -12,7 +12,6 @@ endmacro()
>  REGISTER_SYMLINK("mariadb" "mysql")
>  REGISTER_SYMLINK("mariadb-access" "mysqlaccess")
>  REGISTER_SYMLINK("mariadb-admin" "mysqladmin")
> -REGISTER_SYMLINK("mariadb-backup" "mariabackup")

better not. there're scripts that might be using it.
This is why I suggested `strncmp(my_progname, "mariadb", 7)`
and not `strncmp(my_progname, "maria", 5)` in your PR

>  REGISTER_SYMLINK("mariadb-binlog" "mysqlbinlog")
>  REGISTER_SYMLINK("mariadb-check" "mysqlcheck")
>  REGISTER_SYMLINK("mariadb-client-test-embedded" "mysql_client_test_embedded")
> diff --git a/debian/control b/debian/control
> index 81418c13656..6b225ca4b3d 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -548,7 +548,8 @@ Provides: default-mysql-client,
>            virtual-mysql-client
>  Recommends: libdbd-mariadb-perl | libdbd-mysql-perl,
>              libdbi-perl,
> -            libterm-readkey-perl
> +            libterm-readkey-perl,
> +            mariadb-client-compat

Please, no. deb and rpm packages are different enough already.

don't use different suffixes for them.
Choose either -symlink or -compat (or something else) and use it
consistently both in deb and in rpm.

>  Description: MariaDB database client binaries
>   MariaDB is a fast, stable and true multi-user, multi-threaded SQL database
>   server. SQL (Structured Query Language) is the most popular database query
> @@ -558,6 +559,110 @@ Description: MariaDB database client binaries
>   This package includes the client binaries and the additional tools
>   innotop and mariadb-report (mysqlreport).
> 
> +Package: mariadb-client-compat
> +Architecture: all
> +Depends: mariadb-client
> +Multi-Arch: foreign
> +Description: MySQL compatibility links to mariadb-client binaries/scripts.
> +Conflicts: mariadb-client (<< ${source:Version}),
> +           mariadb-client-10.0,

I wonder, would it work, if you won't set any Conflicts/Breaks, but
only set Depends? It'd be a lot simpler that way (if it'll work).

> --- a/debian/mariadb-backup.install
> +++ b/debian/mariadb-backup.install
> @@ -1,6 +1,4 @@
> -usr/bin/mariabackup

same here. needs mariadb-backup-compat

>  usr/bin/mariadb-backup
>  usr/bin/mbstream
> -usr/share/man/man1/mariabackup.1

>  usr/share/man/man1/mariadb-backup.1
>  usr/share/man/man1/mbstream.1
> diff --git a/debian/mariadb-client.links b/debian/mariadb-client.links
> index 62e3651daf5..8ccce12c609 100644
> --- a/debian/mariadb-client.links
> +++ b/debian/mariadb-client.links
> @@ -2,11 +2,6 @@ usr/bin/mariadb-check usr/bin/mariadb-analyze
>  usr/bin/mariadb-check usr/bin/mariadb-optimize
>  usr/bin/mariadb-check usr/bin/mariadb-repair
>  usr/bin/mariadb-check usr/bin/mariadbcheck
> -usr/bin/mariadb-check usr/bin/mysqlanalyze
> -usr/bin/mariadb-check usr/bin/mysqlcheck
> -usr/bin/mariadb-check usr/bin/mysqloptimize
> -usr/bin/mariadb-check usr/bin/mysqlrepair
> -usr/bin/mariadb-report usr/bin/mysqlreport
>  usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-analyze.1.gz
>  usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-optimize.1.gz
>  usr/share/man/man1/mariadb-check.1.gz usr/share/man/man1/mariadb-repair.1.gz

you forgot to move symlinks to man pages

> diff --git a/debian/mariadb-server-compat.install b/debian/mariadb-server-compat.install
> new file mode 100644
> index 00000000000..36701d8e0c6
> --- /dev/null
> +++ b/debian/mariadb-server-compat.install
> @@ -0,0 +1,12 @@
> +usr/bin/mysqld_multi
> +usr/bin/mysqld_safe
> +usr/bin/mysqld_safe_helper
> +usr/bin/mysql_install_db
> +usr/bin/mysql_upgrade
> +usr/sbin/mysqld
> +usr/share/man/man1/mysqld_multi.1
> +usr/share/man/man1/mysqld_safe.1
> +usr/share/man/man1/mysqld_safe_helper.1

great. do you do the same for rpms?

I didn't see where manpages are moved to -symlinks packages in rpms.

> +usr/share/man/man1/mysql_install_db.1
> +usr/share/man/man1/mysql_upgrade.1
> +usr/share/man/man8/mysqld.8
> diff --git a/debian/mariadb-test-compat.install b/debian/mariadb-test-compat.install
> new file mode 100644
> index 00000000000..7b498d7d1c8
> --- /dev/null
> +++ b/debian/mariadb-test-compat.install
> @@ -0,0 +1 @@
> +usr/share/mysql/mysql-test/mysql-test-run.pl

You forgot mysql-test-run, mysqltest, mysql_client_test,
and their *embedded variants.

But, you know, it's probably not worth a new package at all.
It's the *test* package (not interesting for users, never
used in production, so no immutability requirements).

I suggest to keep those symlinks in the mariadb-test package.
Your other PR will add a warning. And eventually we drop them.
Optional/Recommended/Suggested - sounds like too much toubles
for a *test* package.

> diff --git a/mysql-test/lib/mtr_cases.pm b/mysql-test/lib/mtr_cases.pm
> index fe202279ac7..ad1ac6fdf88 100644
> --- a/mysql-test/lib/mtr_cases.pm
> +++ b/mysql-test/lib/mtr_cases.pm
> @@ -387,14 +387,14 @@ sub collect_suite_name($$)
>      else
>      {
>        my @dirs = my_find_dir(dirname($::glob_mysql_test_dir),
> -                             ["mysql-test/suite", @plugin_suitedirs ],
> +                             ["mariadb-test/suite", "mysql-test/suite", @plugin_suitedirs ],

Eh, no. That is, yes, but not in this set of commits, not in this MDEV