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 --sync missing some recipes #122

Merged
merged 3 commits into from
Sep 8, 2024
Merged

Fix --sync missing some recipes #122

merged 3 commits into from
Sep 8, 2024

Conversation

kylewlacy
Copy link
Member

@kylewlacy kylewlacy commented Sep 7, 2024

This PR fixes an issue where calling brioche build --sync would end up missing some recipes while syncing. The result was that running e.g. brioche install -r eza would end up needing to build the Rust crate, even though the latest version had been built and synced from brioche-packages

This was caused by the function brioche_core::references::recipe_references missing recipes in some cases. Basically, this function wouldn't return child recipes directly, but would skip straight to returning the grandchild recipe dependencies-- effectively skipping a layer of dependencies. If this skipped layer included "expensive" recipes, then that would lead to cases like those seen with eza where the user would end up baking the expensive recipes manually Update: I spent quite a bit of time trying to write a unit test for this issue, but I wasn't able to reproduce it. So, I'm not exactly sure what exact conditions are needed to cause the issue (but I was able to reliably trigger it with the current version of the eza package)

This new change makes recipe syncing slightly slower (~1.5s to ~3s for eza when testing locally), but this new implementation should be more correct

Internally, this change makes it so recipes can either be referenced by hash or referenced directly, which helps when trying to fetch referenced recipes from the database (i.e. no need to fetch a recipe if it's just nested within another recipe)

@kylewlacy kylewlacy merged commit b1b37ab into main Sep 8, 2024
5 checks passed
@kylewlacy kylewlacy deleted the missing-sync branch September 8, 2024 00:15
kylewlacy added a commit that referenced this pull request Sep 8, 2024
kylewlacy added a commit that referenced this pull request Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant