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

_context: use artifact caches from the parent project for junctions #1941

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abderrahim
Copy link
Contributor

This makes a junction use the artifact cache of the parent project before the ones defined for the junction

This was originally done in 24c0de1, but regressed at some point

Fixes #1839

This makes a junction use the artifact cache of the parent project
before the ones defined for the junction

This was originally done in 24c0de1,
but regressed at some point

Fixes #1839
@abderrahim abderrahim force-pushed the abderrahim/junction-remotes branch from fca3645 to cbe213c Compare August 2, 2024 14:39
@juergbi
Copy link
Contributor

juergbi commented Aug 6, 2024

commit 24c0de1 was intentionally reverted as part of https://gitlab.com/BuildStream/buildstream/-/merge_requests/1403.

The remote cache configuration was later redesigned in #1434.

I have to dive deeper to figure out whether this is a correct bug fix, but want to make sure we don't unintentionally change the behavior (away from the agreed design). And we would also need to consider whether this behavior change may break any users.

@abderrahim
Copy link
Contributor Author

I have to dive deeper to figure out whether this is a correct bug fix, but want to make sure we don't unintentionally change the behavior (away from the agreed design).

Yeah, the problem is that there is no agreed design. The mailing list post around #1434 didn't elaborate on this point precisely and my reading of it didn't seem to contradict my understanding of it. However, it was ultimately merged without much discussion.

My research didn't lead me to https://gitlab.com/BuildStream/buildstream/-/merge_requests/1403 (I wasn't on the mailing list back then). I still need to read the whole discussion around that change.

This change brings back the behaviour that we have been using for a long time on buildstream 1.x. I'll try to post on the mailing list with a summary of my research and make a case for this change.

And we would also need to consider whether this behavior change may break any users.

Do you have an idea of who these users might be? In all buildstream projects I worked on, this is the right thing to do.

@nanonyme
Copy link
Contributor

nanonyme commented Aug 6, 2024

@juergbi this has a real monetary impact on any project used as base project that has to pay for bandwidth and discourages such projects from having public artifact caches. It doesn't seem like decision that is matter of gut feeling. Current behaviour is bad for BuildStream community.

@nanonyme
Copy link
Contributor

I just thought of one case that might be affected. Currently child project can have a tiny cache that only fits its own artifacts and I parent project artifacts will be pulled from parent project cache instead. This is disk storage vs network problem. Usually disk storage problem is of course easier to resolve than network. So maybe the ideal solution would be allow-parent-project-artifacts with default value True that can optionally be set to False if you really know what you're doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants