Enables 'rsync' as a copy tool

[nelsonjr]

rsync is much more performant than cp as a means to sync directories, as it will do checks and avoid copying files that already exist in the folder and have the same content.

This reduces reusing a folder copy time from 30s to 1s!

[toote]

This is quite a great idea!

I don't like the implementation for the following reasons:

• it adds an additional backend-specific parameter to the backend_exec
• it adds a backend-specific option
• it breaks single-file restoring (because you changed ${from} to ${from}/.
• that change also broke most tests
• it is only implemented for the restore and not the save
• it is missing documentation
• it is missing tests

I think the alternatives would be:
1- create a specific rsync backend script
2- change the fs backend to just read an environment variable to decide what program to use
3- same as 2, but not only allow rsync or cp but any other executable (with options, though that complicates the code a bit)

I would personally prefer 1 or 3 (in that order), but would be OK with any of those... provided there are tests as well 😛

[toote]

a cleaner way to do this would be:

Suggested change

	if [ -d "$from" ]; then
	if [ "$fs_copy_tool" = "rsync" ]; then
	rsync -a --delete "${from}/." "$to" # copy as folder
	else
	cp -a "${from}/." "$to" # copy as folder
	fi
	else
	if [ "$fs_copy_tool" = "rsync" ]; then
	rsync -a --delete "$from" "$to"
	else
	cp -a "$from" "$to"
	fi
	fi
	if [ "$fs_copy_tool" = "rsync" ]; then
	copy_exec=(rsync -a --delete)
	else
	copy_exec=(cp -a)
	fi
	"${copy_exec[@]}" "${from}" "${to}"

[nelsonjr]

Good points. I will update the code and send another one for review. I did not fork the backend because common logic must be duplicated (and kept in sync).

I like the array trick ;-)

However, regarding this:

it breaks single-file restoring (because you changed ${from} to ${from}/.

If you note, that's under the "if -d," which tests if it is a directory. Only if it's a directory, we add a "/." to it.

[nelsonjr]

it is only implemented for the restore and not the save

On purpose. For save, we always allocate a new (empty) folder, and rsync does not gain over cp for empty folders. We can use the same tool for symmetry, but performance will be the same.

[nelsonjr]

it is missing documentation
it is missing tests

Good points. I will add those!

[nelsonjr]

that change also broke most tests

Hi @toote. I installed bats to run the tests, to both write tests and make sure I did not break any, and it seems it does not run from the HEAD commit (acc044f) in main.

cache-buildkite-plugin.upstream/tests$ ./cache_fs.bats
 ✗ Invalid operation fails silently wtih 255
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist
 ✗ Backend fails if cache folder does not exist
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist
 ✗ Exists on non-existing file fails
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist
 ✗ Exists on empty file fails
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist
 ✗ Exists on existing folder works
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist
 ✗ File exists and can be restored after save
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist
 ✗ Folder exists and can be restored after save
   (from function `setup' in test file cache_fs.bats, line 6)
     `setup() {' failed
   bats: /load.bash does not exist

7 tests, 7 failures

I did not find this load.bash in the source tree.

I'd be happy to write some DEVELOPER.md to help future contributors to know what to expect when making changes.

[toote]

Hi @toote. I installed bats to run the tests

To run tests, we use a buildkite plugin that, in turn, uses docker to run a customized bats image. You should be able to run them with docker run --rm -ti -v $(pwd):/plugin buildkite/plugin-tester (or something like that)