Skip to content

Conversation

@DaanDeMeyer
Copy link
Contributor

We have access to the config object in have_cache() and this argument is specifically intended to be used whenever we pass the default tools tree to have_cache(), so let's just do the check based on config.image in have_cache() itself.

@DaanDeMeyer DaanDeMeyer changed the title Replace check_uid with a "tools" image check in have_cache() Various cache fixes Jan 28, 2025
Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first two look good to me, the third one I don't quite get, since it's mostly doing the same as have_cache with less logging interspersed. The only marked difference is that have_cache also looks at the manifest, which could maybe be optional with a switch or go into a separate function.

@DaanDeMeyer
Copy link
Contributor Author

The first two look good to me, the third one I don't quite get, since it's mostly doing the same as have_cache with less logging interspersed. The only marked difference is that have_cache also looks at the manifest, which could maybe be optional with a switch or go into a separate function.

The thing is, by the time we call reuse_cache(), we've already called have_cache() and cleaned up if needed, so I think it makes more sense for reuse_cache() to behave based on the paths on disk and not check the manifest again.

We have access to the config object in have_cache() and this argument
is specifically intended to be used whenever we pass the default tools
tree to have_cache(), so let's just do the check based on config.image
in have_cache() itself.
By the time reuse_cache() is called, we've already cleaned up old
cached images if needed, so just check if they still exist and reuse
them if they do.
@behrmann behrmann merged commit 3c8ef9a into systemd:main Jan 29, 2025
32 of 35 checks passed
@behrmann
Copy link
Contributor

The thing is, by the time we call reuse_cache(), we've already called have_cache() and cleaned up if needed, so I think it makes more sense for reuse_cache() to behave based on the paths on disk and not check the manifest again.

I don't disagree with the reasoning not check the manifest again, my point was that there's a lot of common logic in have_cache and reuse_cache now, but that can be fixed later, since the same idioms appear elsewhere, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Build fails after config changes(?) because some Directory not empty where mkosi wants to move a tree

2 participants