You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2022/08/31 15:15:13 UTC
[GitHub] [buildstream] ssssam opened a new pull request, #1746: Fail early if `buildbox-run` is present but non-functional
ssssam opened a new pull request, #1746:
URL: https://github.com/apache/buildstream/pull/1746
If buildbox-run is missing, BuildStream falls back to "dummy" sandbox
which allows the program to start, and causes build command execution
to fail with a "missing sandbox" error.
Previously, if buildbox-run was present but broken (e.g. segfaulting
on startup), the same fallback codepath would be taken. This lead to
serious breakage in the buildstream-docker-images going undetected:
https://gitlab.com/BuildStream/buildstream-docker-images/-/issues/50
With this change, a broken buildbox-run leads to BuildStream failing
on startup with an error such as this:
Error instantiating platform: buildbox-run exited with code 5. Output: I am broken!
--
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
[GitHub] [buildstream] abderrahim commented on a diff in pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
abderrahim commented on code in PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#discussion_r959841234
##########
src/buildstream/_platform/platform.py:
##########
@@ -44,16 +44,16 @@ def _setup_sandbox(self):
# Try to setup buildbox-run sandbox, otherwise fallback to the dummy sandbox.
try:
self._check_sandbox(SandboxBuildBoxRun)
- except (SandboxError, utils.ProgramNotFoundError):
+ except (SandboxUnsupportedError, utils.ProgramNotFoundError):
pass
def _check_sandbox(self, Sandbox):
Sandbox._dummy_reasons = []
try:
Sandbox.check_available()
- except SandboxError as Error:
+ except SandboxUnsupportedError as Error:
Review Comment:
The reason I didn't modify it in #1436 is that I wanted to have it backportable to bst-1 (but turns out bst-1 didn't have a dummy sandbox at all, so whatever).
There are actually two different checks `Sandox.check_available()` and `Sandbox._check_sandbox_config()` (is there a reason why the former is "public"?) and we want to cache the result of the former. That's essentially why dummy_reasons exist.
What I would recommend is to do the caching in the `Sandbox` class itself, such that calling `_check_sandbox_config()` will raise the newly introduced `SandboxUnsupportedError` if buildbox-run isn't available before actually checking the config (and possibly drop `Sandox.check_available()` altogether if it's not needed for your new check?)
--
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
[GitHub] [buildstream] ssssam commented on pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
ssssam commented on PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#issuecomment-1233124650
The fedora-36 test suite failures actually show that this PR is working as intended: in the current testsuite image, registry.gitlab.com/buildstream/buildstream-docker-images/testsuite-fedora:36-master-533491591, buildbox-run segfaults on startup due to https://gitlab.com/BuildStream/buildstream-docker-images/-/issues/50
We can update the testsuite image tag to fix that.
--
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
[GitHub] [buildstream] ssssam commented on a diff in pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
ssssam commented on code in PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#discussion_r975323102
##########
src/buildstream/_platform/platform.py:
##########
@@ -44,16 +44,16 @@ def _setup_sandbox(self):
# Try to setup buildbox-run sandbox, otherwise fallback to the dummy sandbox.
try:
self._check_sandbox(SandboxBuildBoxRun)
- except (SandboxError, utils.ProgramNotFoundError):
+ except (SandboxUnsupportedError, utils.ProgramNotFoundError):
pass
def _check_sandbox(self, Sandbox):
Sandbox._dummy_reasons = []
try:
Sandbox.check_available()
- except SandboxError as Error:
+ except SandboxUnsupportedError as Error:
Review Comment:
This is renamed to _setup in 1cd18ff3c5b5c5c0d190bcfe36842f730c6a5ba0
--
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
[GitHub] [buildstream] nanonyme commented on pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#issuecomment-1399336709
@ssssam could you please rebase 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
[GitHub] [buildstream] gtristan commented on a diff in pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#discussion_r959813893
##########
src/buildstream/_platform/platform.py:
##########
@@ -44,16 +44,16 @@ def _setup_sandbox(self):
# Try to setup buildbox-run sandbox, otherwise fallback to the dummy sandbox.
try:
self._check_sandbox(SandboxBuildBoxRun)
- except (SandboxError, utils.ProgramNotFoundError):
+ except (SandboxUnsupportedError, utils.ProgramNotFoundError):
pass
def _check_sandbox(self, Sandbox):
Sandbox._dummy_reasons = []
try:
Sandbox.check_available()
- except SandboxError as Error:
+ except SandboxUnsupportedError as Error:
Review Comment:
Also in the line below we are collecting this weird reasons list of strings, it appears to be the last place justifying the existence of this `Sandbox._dummy_reasons` variable, any chance we can take this opportunity to get rid of it ? (Not sure we need this similar variable `Platform.dummy_reasons` either).
--
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
[GitHub] [buildstream] gtristan commented on a diff in pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#discussion_r960635674
##########
src/buildstream/_platform/platform.py:
##########
@@ -44,16 +44,16 @@ def _setup_sandbox(self):
# Try to setup buildbox-run sandbox, otherwise fallback to the dummy sandbox.
try:
self._check_sandbox(SandboxBuildBoxRun)
- except (SandboxError, utils.ProgramNotFoundError):
+ except (SandboxUnsupportedError, utils.ProgramNotFoundError):
pass
def _check_sandbox(self, Sandbox):
Sandbox._dummy_reasons = []
try:
Sandbox.check_available()
- except SandboxError as Error:
+ except SandboxUnsupportedError as Error:
Review Comment:
It should be made private generally, so as not to take up any public namespace (In this instance it’s not very important given that the user does not subclass sandboxes).
One underscore for shared internal API, two underscores for module internal details (two underscores are only used in these public facing classes, where the distinction between “private” and “internal” is significant).
--
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
[GitHub] [buildstream] ssssam commented on pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
ssssam commented on PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#issuecomment-1234330044
The fedora-missing-deps test is failing now because in the testsuite-fedora:minimal image, there is a `buildbox-run` program which fails to start because there is no `bwrap`.
That seems like a bug in the testsuite-fedora:minimal image to me, I will submit a patch at buildstream-docker-images to remove buildbox-run altogether from the minimal image.
--
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
[GitHub] [buildstream] ssssam commented on a diff in pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
ssssam commented on code in PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#discussion_r960609588
##########
src/buildstream/_platform/platform.py:
##########
@@ -44,16 +44,16 @@ def _setup_sandbox(self):
# Try to setup buildbox-run sandbox, otherwise fallback to the dummy sandbox.
try:
self._check_sandbox(SandboxBuildBoxRun)
- except (SandboxError, utils.ProgramNotFoundError):
+ except (SandboxUnsupportedError, utils.ProgramNotFoundError):
pass
def _check_sandbox(self, Sandbox):
Sandbox._dummy_reasons = []
try:
Sandbox.check_available()
- except SandboxError as Error:
+ except SandboxUnsupportedError as Error:
Review Comment:
Ok, updated based on that feedback. I renamed SandboxBuildBoxRun.check_available() to `setup()` as that's more correct based on what it does. I kept it public as, the whole _sandboxbuildboxrun module is private API, and setup() is called from outside the module. (I may be misunderstanding how the rules work in Bst though).
--
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
[GitHub] [buildstream] ssssam commented on pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by "ssssam (via GitHub)" <gi...@apache.org>.
ssssam commented on PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#issuecomment-1400286049
sure, rebased
--
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
[GitHub] [buildstream] gtristan commented on a diff in pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#discussion_r959809136
##########
src/buildstream/_platform/platform.py:
##########
@@ -44,16 +44,16 @@ def _setup_sandbox(self):
# Try to setup buildbox-run sandbox, otherwise fallback to the dummy sandbox.
try:
self._check_sandbox(SandboxBuildBoxRun)
- except (SandboxError, utils.ProgramNotFoundError):
+ except (SandboxUnsupportedError, utils.ProgramNotFoundError):
pass
def _check_sandbox(self, Sandbox):
Sandbox._dummy_reasons = []
try:
Sandbox.check_available()
- except SandboxError as Error:
+ except SandboxUnsupportedError as Error:
Review Comment:
Let’s fix the `Error` variable and call it `e` if we still need it after correcting the `raise` statement below
--
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
[GitHub] [buildstream] ssssam commented on pull request #1746: Fail early if `buildbox-run` is present but non-functional
Posted by GitBox <gi...@apache.org>.
ssssam commented on PR #1746:
URL: https://github.com/apache/buildstream/pull/1746#issuecomment-1295223626
Latest [buildstream-docker-images](https://gitlab.com/BuildStream/buildstream-docker-images) should include that fix to fedora-minimal image ([#206](https://gitlab.com/BuildStream/buildstream-docker-images/-/merge_requests/206)).
However, pipelines there are currently failing due to Fedora 37 update in progress. Lets consider this PR blocked on https://github.com/apache/buildstream/pull/1780, itself blocked on a successful pipeline at https://gitlab.com/BuildStream/buildstream-docker-images/-/pipelines
--
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