You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by "nanonyme (via GitHub)" <gi...@apache.org> on 2023/08/22 15:16:38 UTC
[GitHub] [buildstream] nanonyme opened a new pull request, #1864: Raise exception with more information upon plugin bug
nanonyme opened a new pull request, #1864:
URL: https://github.com/apache/buildstream/pull/1864
(no comment)
--
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] Raise exception with more information upon plugin bug [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#issuecomment-1769786374
Yes thankyou ! Sorry again for leading you down a rabbit hole
--
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 #1864: Raise exception with more information upon plugin bug
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#discussion_r1318088984
##########
src/buildstream/source.py:
##########
@@ -749,9 +749,12 @@ def mark_download_url(self, url: str, *, primary: bool = True) -> None:
if primary:
expected_alias = _extract_alias(url)
- assert (
- self.__expected_alias is None or self.__expected_alias == expected_alias
- ), "Primary URL marked twice with different URLs"
+ if self.__expected_alias is not None and self.__expected_alias != expected_alias:
+ raise BstError(
Review Comment:
By the way, if anything... there are a number of places where we explicitly use `assert` for this purpose, but it would be desirable to change *all* of these cases for an explicit (not `BstError` inheriting) exception, because there are conceivable ways to run python code with `assert` lines disabled (although that doesn't really ever happen in practice as far as I know).
--
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 #1864: Raise exception with more information upon plugin bug
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#discussion_r1325291666
##########
src/buildstream/source.py:
##########
@@ -749,9 +749,12 @@ def mark_download_url(self, url: str, *, primary: bool = True) -> None:
if primary:
expected_alias = _extract_alias(url)
- assert (
- self.__expected_alias is None or self.__expected_alias == expected_alias
- ), "Primary URL marked twice with different URLs"
+ if self.__expected_alias is not None and self.__expected_alias != expected_alias:
+ raise BstError(
Review Comment:
Unless we're going to make a sweeping change that changes all of our asserts where we intend to result in a BUG message with accompanying stack trace, I'm not happy with adding the new `Bug` exception.
I.e. it will be easily forgotten and will not be an obvious matter of policy.
Also, the problem with this we have to realize is that unfortunately, BuildStream has dependencies.
So if one wanted to run BuildStream without assertions (e.g. using `python3 -O ...` or with `PYTHONOPTIMIZE=1`), we would then be ignoring any assert statements from any of our dependencies... I'm happy to be convinced otherwise, but I think in the wild west of python dependencies, nobody really cares about supporting running with assertions disabled :-/
For this patch, I think you really just wanted to add some context to the message, it would probably be best to just update the message in the existing assertion.
I'm sorry for not having thought this through more deeply when you originally submitted 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 #1864: Raise exception with more information upon plugin bug
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan commented on code in PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#discussion_r1318087454
##########
src/buildstream/source.py:
##########
@@ -749,9 +749,12 @@ def mark_download_url(self, url: str, *, primary: bool = True) -> None:
if primary:
expected_alias = _extract_alias(url)
- assert (
- self.__expected_alias is None or self.__expected_alias == expected_alias
- ), "Primary URL marked twice with different URLs"
+ if self.__expected_alias is not None and self.__expected_alias != expected_alias:
+ raise BstError(
Review Comment:
In this case we explicitly do not want a `BstError`, as these are intended to communicate a clear reason for a failure to the user so that the user can potentially address the problem.
In this case we want it to bubble up as a regular exception, so that the core will treat it as a `BUG` and print a stack trace, which is more helpful for plugin authors to address bugs in their plugins.
--
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] Raise exception with more information upon plugin bug [buildstream]
Posted by "gtristan (via GitHub)" <gi...@apache.org>.
gtristan merged PR #1864:
URL: https://github.com/apache/buildstream/pull/1864
--
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 a diff in pull request #1864: Raise exception with more information upon plugin bug
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#discussion_r1324917012
##########
src/buildstream/source.py:
##########
@@ -749,9 +749,12 @@ def mark_download_url(self, url: str, *, primary: bool = True) -> None:
if primary:
expected_alias = _extract_alias(url)
- assert (
- self.__expected_alias is None or self.__expected_alias == expected_alias
- ), "Primary URL marked twice with different URLs"
+ if self.__expected_alias is not None and self.__expected_alias != expected_alias:
+ raise BstError(
Review Comment:
Added a new exception called Bug.
--
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 a diff in pull request #1864: Raise exception with more information upon plugin bug
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on code in PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#discussion_r1326319322
##########
src/buildstream/source.py:
##########
@@ -749,9 +749,12 @@ def mark_download_url(self, url: str, *, primary: bool = True) -> None:
if primary:
expected_alias = _extract_alias(url)
- assert (
- self.__expected_alias is None or self.__expected_alias == expected_alias
- ), "Primary URL marked twice with different URLs"
+ if self.__expected_alias is not None and self.__expected_alias != expected_alias:
+ raise BstError(
Review Comment:
Fixed.
--
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] Raise exception with more information upon plugin bug [buildstream]
Posted by "nanonyme (via GitHub)" <gi...@apache.org>.
nanonyme commented on PR #1864:
URL: https://github.com/apache/buildstream/pull/1864#issuecomment-1768513182
@gtristan so is this now okay after your requested changes?
--
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