You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "juergbi (via GitHub)" <gi...@apache.org> on 2025/05/23 10:46:50 UTC
[PR] Add support for REAPI execution from the sandbox [buildstream]
juergbi opened a new pull request, #2014:
URL: https://github.com/apache/buildstream/pull/2014
Fixes #1985.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#issuecomment-2970926053
> I'm adding support for action-cache-service, which then enables use of the REAPI socket with storage-service. That branch is not yet tested (or documented), though, and I think it's ok to first merge this and then the action-cache-service addition.
Yeah, this is fine. With the clear error message here, we can land this even if it doesn't support the cache storage-service.
I'll test the action-cache-service branch in the mean time
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#discussion_r2146793679
##########
src/buildstream/_cas/casdprocessmanager.py:
##########
@@ -45,9 +45,9 @@
#
# Minimum required version of buildbox-casd
#
-_REQUIRED_CASD_MAJOR = 0
-_REQUIRED_CASD_MINOR = 0
-_REQUIRED_CASD_MICRO = 58
+_REQUIRED_CASD_MAJOR = 1
+_REQUIRED_CASD_MINOR = 2
Review Comment:
I think we can have some cleanups of fallback code paths now that we require buildbox 1.2. That shouldn't block this pull request, but it should be nice to have it done before the next release.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#issuecomment-2919895489
I feel it's still not ready. While it's good enough, it depends on unreleased buildbox and there are a few corner cases such as https://github.com/apache/buildstream/pull/2014#issuecomment-2911646294 above that need to be handled.
I'd say let's leave it till 2.6. I think that's also @juergbi's opinion as this is still marked as draft.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on code in PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#discussion_r2145495496
##########
tests/integration/project/elements/sandbox/remote-apis-socket.bst:
##########
@@ -0,0 +1,13 @@
+kind: manual
+
+depends:
+ - filename: base.bst
+ type: build
+
+sandbox:
+ remote-apis-socket:
+ path: /tmp/reapi.sock
+
+config:
+ build-commands:
+ - test -e /tmp/reapi.sock
Review Comment:
Can't we test that it's a socket instead?
```suggestion
- test -S /tmp/reapi.sock
```
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#issuecomment-2919163551
Generally like this patch overall... although it is still in draft, do we want this in 2.5 ?
Should we add an integration test for exercising `recc` ? Would be pretty cool if we had an example for this in the canonical format too ?
I'd be happy to add a tested example based on the [recc demo](https://gitlab.com/tristanvb/buildstream-recc-demo/) to sweeten the pot if we think we're ready to go ahead with this...
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#discussion_r2146767568
##########
tests/integration/project/elements/sandbox/remote-apis-socket.bst:
##########
@@ -0,0 +1,13 @@
+kind: manual
+
+depends:
+ - filename: base.bst
+ type: build
+
+sandbox:
+ remote-apis-socket:
+ path: /tmp/reapi.sock
+
+config:
+ build-commands:
+ - test -e /tmp/reapi.sock
Review Comment:
Good suggestion, changed.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "abderrahim (via GitHub)" <gi...@apache.org>.
abderrahim commented on PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#issuecomment-2911646294
I tested this, and recc inside the sandbox is complaining
```
2025-05-27T08:22:40.382+0000 [218:140519756722176] [buildboxcommon_grpcretrier.cpp:134] [ERROR] ActionCache.GetActionResult() failed with: 12: Action Cache service is not enabled for instance ""
2025-05-27T08:22:40.383+0000 [218:140519756722176] [executioncontext.cpp:467] [ERROR] Error while querying action cache at "unix:/tmp/casd.sock": 12: Action Cache service is not enabled for instance ""
2025-05-27T08:22:40.385+0000 [218:140519756722176] [buildboxcommon_grpcretrier.cpp:134] [ERROR] Execution.Execute() failed with: 12: Execute: 12: Action Cache service is not enabled for instance ""
2025-05-27T08:22:40.386+0000 [218:140519756722176] [executioncontext.cpp:832] [ERROR] Error while calling `Execute()` on "unix:/tmp/casd.sock": 12: Execute: 12: Action Cache service is not enabled for instance ""
```
I dug a bit and it is due to the fact that I'm using a cache storage service. Disabling it works fine.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi merged PR #2014:
URL: https://github.com/apache/buildstream/pull/2014
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on code in PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#discussion_r2120607243
##########
src/buildstream/sandbox/_config.py:
##########
@@ -66,15 +74,15 @@ def __init__(
# Returns:
# A dictionary representation of this SandboxConfig
#
- def to_dict(self) -> Dict[str, Union[str, int]]:
+ def to_dict(self) -> Dict[str, Union[str, int, bool]]:
# Assign mandatory portions of the sandbox configuration
#
# /!\ No additional mandatory members can ever be added to
# the sandbox configuration, as that would result in
# breaking cache key stability.
#
- sandbox_dict: Dict[str, Union[str, int]] = {"build-os": self.build_os, "build-arch": self.build_arch}
+ sandbox_dict: Dict[str, Union[str, int, bool]] = {"build-os": self.build_os, "build-arch": self.build_arch}
Review Comment:
Yes, there was indeed an additional boolean config option in a previous iteration. Will remove this.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#issuecomment-2970762558
> I dug a bit and it is due to the fact that I'm using a cache storage service. Disabling it works fine.
I've added a check for this to fail the build with a clear error message. In https://github.com/apache/buildstream/tree/jbilleter/action-cache-service, I'm adding support for `action-cache-service`, which then enables use of the REAPI socket with `storage-service`. That branch is not yet tested (or documented), though, and I think it's ok to first merge this and then the `action-cache-service` addition.
I've also added a minimal test to this branch.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "juergbi (via GitHub)" <gi...@apache.org>.
juergbi commented on PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#issuecomment-2929736659
Leaving it till 2.6 is fine with me. I anyway expect typical use cases to require #1991 and that's also still a draft.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for REAPI execution from the sandbox [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #2014:
URL: https://github.com/apache/buildstream/pull/2014#discussion_r2113782461
##########
src/buildstream/sandbox/_config.py:
##########
@@ -66,15 +74,15 @@ def __init__(
# Returns:
# A dictionary representation of this SandboxConfig
#
- def to_dict(self) -> Dict[str, Union[str, int]]:
+ def to_dict(self) -> Dict[str, Union[str, int, bool]]:
# Assign mandatory portions of the sandbox configuration
#
# /!\ No additional mandatory members can ever be added to
# the sandbox configuration, as that would result in
# breaking cache key stability.
#
- sandbox_dict: Dict[str, Union[str, int]] = {"build-os": self.build_os, "build-arch": self.build_arch}
+ sandbox_dict: Dict[str, Union[str, int, bool]] = {"build-os": self.build_os, "build-arch": self.build_arch}
Review Comment:
Is this outdated perhaps ? Maybe there was an added boolean here in a previous iteration of the patch ?
I think here there is only the possibility of a new string being serialized in the dict.
--
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: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org