Tools: Remove dependencies installed by make tools

[FirePing32]

Signed-off-by: Prakhar Gurunani prakhargurunani@gmail.com

Description

The script removes the tools and dependencies installed by make tools and also removes the symlinks between them.

Related Issue(s)

fixes: #14288

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on the CI
• Documentation was added or is not required

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[FirePing32]

Should this script be added somewhere in the docs?

[frouioui]

Should this script be added somewhere in the docs?

I think it makes the most sense to me to have a new rule in the Makefile that calls this script. The same way we have make tools, we could have make clean_tools.

[frouioui]

Looks good to me besides that small potential issue with protoc's symlink. Thank you for this contribution.

I have tested this on a remote machine with a clean clone of vitess running make tools and the new script added by this PR.

[mattlord]

I think that this is a good idea. I'm curious where you get the 9GiB assessment from?

The files are here: https://github.com/vitessio/vitess-resources/releases/

And at least for me:

❯ du -csh "${VTROOT}/dist"
 74M	/Users/matt/git/vitess/dist
 74M	total

Do you really see the disk usage go down by ~ 9GiB after running this script? If so, then I agree that it makes it all the more useful 🙂. I'd also triple check, however, that we're not removing things we did not intend to.

[mattlord]

Nice work on this, @FirePing32 ! I only had a few minor comments. Let me know what you think and I can come back and re-review quickly at any time.

[mattlord]

@FirePing32 Sorry, one last thing. We should address the shellcheck warnings — these should have been surfaced by the pre-commit hook .git/hooks/pre-commit which then ends up running misc/git/hooks/shellcheck. They are all minor variable quoting issues:

❯ shellcheck tools/remove_dependencies.sh

In tools/remove_dependencies.sh line 40:
        rm -rf $dist
               ^---^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        rm -rf "$dist"


In tools/remove_dependencies.sh line 49:
        rm -rf $dist
               ^---^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        rm -rf "$dist"


In tools/remove_dependencies.sh line 60:
        Darwin) local platform=darwin; local ext=zip;;
                                             ^-^ SC2034 (warning): ext appears unused. Verify use (or export if used externally).


In tools/remove_dependencies.sh line 81:
        rm -rf $dist
               ^---^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        rm -rf "$dist"


In tools/remove_dependencies.sh line 94:
        rm -rf $dist
               ^---^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        rm -rf "$dist"


In tools/remove_dependencies.sh line 122:
        rm -rf $dist
               ^---^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
        rm -rf "$dist"


In tools/remove_dependencies.sh line 139:
    uninstall_etcd $ETCD_VER "$VTROOT/dist/etcd"
                   ^-------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
    uninstall_etcd "$ETCD_VER" "$VTROOT/dist/etcd"

For more information:
  https://www.shellcheck.net/wiki/SC2034 -- ext appears unused. Verify use (o...
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

If we don't do this then it will show up for other people when modifying related files.

[mattlord]

Just one left:

❯ shellcheck tools/remove_dependencies.sh

In tools/remove_dependencies.sh line 60:
        Darwin) local platform=darwin; local ext=zip;;
                                             ^-^ SC2034 (warning): ext appears unused. Verify use (or export if used externally).

For more information:
  https://www.shellcheck.net/wiki/SC2034 -- ext appears unused. Verify use (o...

Did you intend to use that $ext variable or should we remove it?

[FirePing32]

I guess we don't need that now.

[mattlord]

As part of a minimal manual test, I did this:

make tools

make clean_tools

make clean_tools fails:

❯ make clean_tools
make: source: No such file or directory
make: *** [clean_tools] Error 1

We cannot use source as the Makefile is not a shell script:

diff --git a/Makefile b/Makefile
index b3198536f7..43b6906087 100644
--- a/Makefile
+++ b/Makefile
@@ -393,7 +393,7 @@ tools:
        ./bootstrap.sh

 clean_tools:
diff --git a/Makefile b/Makefile
index b3198536f7..22193f8b4b 100644
--- a/Makefile
+++ b/Makefile
@@ -393,7 +393,7 @@ tools:
        ./bootstrap.sh

 clean_tools:
-       source build.env
+       bash ./build.env
        ./tools/remove_dependencies.sh

 minimaltools:

But then it still fails:

❯ make clean_tools
## local system details...
## platform: Darwin target:arm64 OS: darwin23
./tools/remove_dependencies.sh: line 131: PROTOC_VER: unbound variable
make: *** [clean_tools] Error 1

And even when manually sourcing it:

❯ make clean_tools
## local system details...
## platform: Darwin target:arm64 OS: darwin23
Removing protoc...
Removing zookeeper...
Removing etcd...
rm: /Users/matt/git/vitess/bin/etcd: No such file or directory
make: *** [clean_tools] Error 1

Can you please run some basic manual tests to ensure that this is working as intended?

[mattlord]

For the main issue, you could do this or use ${VTROOT}/build.env if you prefer:

diff --git a/Makefile b/Makefile
index b3198536f7..625a28baac 100644
--- a/Makefile
+++ b/Makefile
@@ -393,7 +393,6 @@ tools:
        ./bootstrap.sh

 clean_tools:
-       source build.env
        ./tools/remove_dependencies.sh

 minimaltools:
diff --git a/tools/remove_dependencies.sh b/tools/remove_dependencies.sh
index 32a6f56496..f47d29f68f 100755
--- a/tools/remove_dependencies.sh
+++ b/tools/remove_dependencies.sh
@@ -18,6 +18,8 @@

 set -euo pipefail

+source "$(dirname "${BASH_SOURCE[0]:-$0}")/../build.env"
+
 function fail() {
        echo "ERROR: ${1}"
        exit 1

[mattlord]

For the secondary issue:

    if [ -f "$dist/etcd-${version}-${platform}-${target}/etcd" ]; then
        unlink "$dist/etcd-${version}-${platform}-${target}/etcd"
        rm "$VTROOT/bin/etcd"
    fi

We're removing a file there that we have not confirmed exists. We can either check that it exists, or using rm -f here is also fine IMO.

[mattlord]

Thank you for the contribution, @FirePing32 ! I tested it and it all seems to be working as expected. ❤️

[mattlord]

I'm going to override and merge because this new code is not part of any tests -- and there are two workflows which are out to pasture with the Expected ... state. The only thing you can do in those cases is push another empty commit but that is not needed in this case.