Correctly detect changes in rootdir

[danstiner]

When using buildah bud --layers ... with copy of the root dir (eg COPY . /app), cached builds will skip the COPY layer even if files have in fact changed.

This is because resolveModifiedTime bails and says there are notifications if the rootdir is being checked. I don't see any reason we can't check the rootdir except for the custom string slicing to compute relative paths. I replaced that with a call to filepath.Rel() and things are working as expected.

I believe this may fix containers/podman#2544

I used the following script as a repro that changes the content of a file and re-builds with --layers.

#!/bin/sh
set -euo pipefail

BUILDAH=buildah

rm -r test || true
mkdir test

echo "A" > test/file
$BUILDAH bud -t layer-caching-experiment:A .

echo "B" > test/file
$BUILDAH bud --layers -t layer-caching-experiment:B-cached .
$BUILDAH bud -t layer-caching-experiment:B .

echo "Build with file content A"
podman run --rm layer-caching-experiment:A cat /app/test/file
echo "Build with file content B (cached)"
podman run --rm layer-caching-experiment:B-cached cat /app/test/file
echo "Build with file content B (no cache)"
podman run --rm layer-caching-experiment:B cat /app/test/file

Expected output shows a cached build picking up the changes to the file:

Build with file content A
A
Build with file content B (cached)
B
Build with file content B (no cache)
B

Actual output as of d43787b:

Build with file content A
A
Build with file content B (cached)
A
Build with file content B (no cache)
B

[rh-atomic-bot]

Can one of the admins verify this patch?
I understand the following commands:

• bot, add author to whitelist
• bot, test pull request
• bot, test pull request once

[rhatdan]

bot, add author to whitelist

[rhatdan]

Thanks, @danstiner
Tests are failing
@TomSweeneyRedHat @umohnani8 @nalind PTAL

[danstiner]

Looks like I missed signing off the commit, did that and force pushed.

[TomSweeneyRedHat]

The code LGTM, but would like a head nod from @nalind as I'm having problems testing this. I'm running into problems with Podman after I vendor this in, but I think it's self-induced and not related to this PR.

[rhatdan]

@danstiner Could you add the tests to make sure we don't regress this.

[danstiner]

Happy to either as part of this PR or as a follow-up. May take a couple days depending how busy I am.

[nalind]

This approach looks correct to me. Nice catch!

[umohnani8]

LGTM

[rhatdan]

@rh-atomic-bot r+
@danstiner I am going to merge, could you add a new PR with some tests, Thanks for this patch.

[rh-atomic-bot]

📌 Commit acee3b3 has been approved by rhatdan

[rh-atomic-bot]

⌛ Testing commit acee3b3 with merge 4bd9f81...

[rh-atomic-bot]

☀️ Test successful - status-papr, status-travis
Approved by: rhatdan
Pushing 4bd9f81 to master...