gtristan commented on a change in pull request #1450:
URL: https://github.com/apache/buildstream/pull/1450#discussion_r586181133
##########
File path: src/buildstream/_artifact.py
##########
@@ -550,11 +550,7 @@ def get_dependency_artifact_names(self):
# Returns:
# (bool): Whether artifact is in local cache
#
- def cached(self):
-
- if self._cached is not None:
- return self._cached
-
+ def query_cache(self):
Review comment:
This is rather orthogonal to the patch, but I think we need to cleanup
how this function behaves differently depending on global values decided in the
`Context`.
I don't feel like it is safe to have such a deep function call behaving
differently *whole sale* based on user configuration, and think rather we
should be passing these values in as arguments when checking what *"domains"*
of an artifact are locally cached (i.e. is only the ref cached, are the files
available locally, etc).
* `Context.require_artifact_files`
This is only ever set to `False` by `Stream` in the case that remote
execution is enabled
* `Context.require_artifact_directories`
This is unconditionally `True` all the time, maybe this historically meant
something, but we forgot to remove this boolean once things changed such that
it is always true (at least grep seems to tell me nothing ever sets this value
to `False`)
* `Element._artifact_files_required()`
Even here I feel like it may happen that an `Element` which does evaluate
to `False` for this function (i.e. it *"requires artifact files"*) could
justifiably decide to ask the `Artifact` whether the files are cached or not
We can resolve this separately from the branch, but as I am reading it this
jumps out at me and strikes me as problematic so just raising it here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
