Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix regression of mktree not always rebuilding #2726

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Oct 17, 2023

Commit d4e808e added cmake caching for mktree so that we don't rebuild the tree on each "make check" invocation. However, this means we can now easily miss some test dependencies being updated since we don't depend on everything, e.g. the data/ dir.

To fix this, instead of adding each and every file and directory as a dependency of the mktree.output command, just rebuild the tree every time. The base image (made with DNF) is already cached anyway, and the added overhead is tiny and outweighs the (potential) engineering hours lost due to this.

The funny thing is, this was already fixed once in 2019: fc05045

Sometimes you've just gotta fix stuff twice before it sticks!

Fixes: #2725

Commit d4e808e added cmake caching for
mktree so that we don't rebuild the tree on each "make check"
invocation.  However, this means we can now easily miss some test
dependencies being updated since we don't depend on everything, e.g. the
data/ dir.

To fix this, instead of adding each and every file and directory as a
dependency of the mktree.output command, just rebuild the tree every
time.  The base image (made with DNF) is already cached anyway, and the
added overhead is tiny and outweighs the (potential) engineering hours
lost due to this.

The funny thing is, this was already fixed once in 2019:
fc05045

Sometimes you've just gotta fix stuff twice before it sticks!

Fixes: rpm-software-management#2725
@dmnks dmnks force-pushed the always-rebuild-tests branch from 757cd17 to d94a642 Compare October 17, 2023 11:49
@dmnks
Copy link
Contributor Author

dmnks commented Oct 17, 2023

This turned out to be a pretty recent regression (commit d4e808e) 😅

@pmatilai
Copy link
Member

Oh. Yep, I even kinda remember seeing that commit. Didn't ring a bell today though 😆

/me feels silly for not investigating at least that much...

I indeed had a feeling that this was a pretty recent thing, but then something like this can go unnoticed for some time.

@pmatilai pmatilai merged commit 12b073b into rpm-software-management:master Oct 17, 2023
1 check passed
@dmnks
Copy link
Contributor Author

dmnks commented Oct 17, 2023

Nah, no worries. It got buried in the other refactorings in that PR anyway. I also didn't realize this at first even though it's kinda obvious.

The timing of you finding this is actually just perfect as it makes the other PR for OCI images that I'm finishing up (for a while now, lol) a little bit simpler.

@dmnks dmnks added the test Testsuite-related label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Testsuite-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test-data not updated in test-suite
2 participants