abderrahim commented on code in PR #1994:
URL: https://github.com/apache/buildstream/pull/1994#discussion_r1966478643
##########
src/buildstream/element.py:
##########
@@ -1272,6 +1272,22 @@ def _get_cache_key(self, strength=_KeyStrength.STRONG):
else:
return self.__weak_cache_key
+ # _get_cas_digest():
+ #
+ # Returns the CAS digest
+ #
+ # Returns:
+ # (str|None): The CAS digest for this Element, or None
+ #
+ # None is returned if no CAS artifact is available for this element.
+ #
+ def _get_cas_digest(self):
+ if self.__artifact is None:
+ return None
+ if not self.__artifact.query_cache():
Review Comment:
Is this still needed? Now that you're correctly querying the cache at the
beginning?
##########
src/buildstream/storage/_casbaseddirectory.py:
##########
@@ -529,6 +529,10 @@ def is_dir_in(entry: _IndexEntry, directory:
CasBasedDirectory) -> bool:
self.__invalidate_digest()
+ def get_cas_digest(self):
Review Comment:
This adds a public method to a private class, and it feels weird.
If we want to add a public method, it should be added to the base
`Directory` class and be implemented by both subclasses.
That said, I feel we should not add public method at this time as nothing is
to be exposed to plugins. We can consider adding the public method later in
#1991 and just use the internal methods for now.
##########
src/buildstream/_frontend/widget.py:
##########
@@ -437,6 +437,14 @@ def show_pipeline(self, dependencies, format_):
runtime_deps = [e._get_full_name() for e in
element._dependencies(_Scope.RUN, recurse=False)]
line = p.fmt_subst(line, "runtime-deps",
_yaml.roundtrip_dump_string(runtime_deps).rstrip("\n"))
+ # CAS Digest
+ if "%{cas-digest" in format_:
+ cas_digest = element._get_cas_digest()
Review Comment:
I kinda think we should implement this here for the moment. Call
`element._get_artifact()` to get the artifact, and try to format the output
here.
Later, we could have a better API to expose to the plugins, and we can use
it here too.
##########
man/bst-show.1:
##########
@@ -45,6 +45,7 @@ Show elements in the pipeline
%{deps} A list of all dependencies
%{build-deps} A list of build dependencies
%{runtime-deps} A list of runtime dependencies
+ %{cas-digest} The CAS digest
Review Comment:
A bit of bike shedding:
* "The CAS digest" is too short to explain what this does. The CAS digest of
what exactly?
* I feel that `cas-digest` is too generic. This is actually the cas digest
of the built artifact, we could (maybe not now, but in the future) have cas
digests of other things (like the source code, or the buildtree if we have one)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
