remove chronos dependency #38
Conversation
The `chronos` backend currently does not work as it relies on nesting `waitFor` calls, which is not supported by chronos itself - as such, this PR disables the backend in testing and prints a recommendation to users to avoid it until the problem has been resolved. faststreams used without `async` continues to work as normal, which also is the most common way to use it - the dependency on chronos in the nimble file is unattractive from a user perspective since it also leads to the the downloading of bearssl and other deps. Hopefully, by the time the chronos backend can be used, we can also specify conditional dependencies in nimble files.
| # TODO asyncdispatch backend is broken / untested | ||
| for backend in ["-d:asyncBackend=none", "-d:asyncBackend=chronos"]: | ||
| # TODO chronos backend uses nested waitFor which is not supported | ||
| for backend in ["-d:asyncBackend=none"]: |
There was a problem hiding this comment.
Valid Chronos usage is possible without invoking the nested waitFor, so I don't welcome the removal of these tests, but perhaps they are not easy to setup now that we don't have the dependency?
There was a problem hiding this comment.
adding nimble install chronos in the ci scripts is enough to keep the tests enabled - my concern is mostly that the tests will start failing
There was a problem hiding this comment.
ie they'll start failing because of changes in chronos which will make it harder to address non-chronos issues in faststreams (since faststreams is relying on effectively UB in chronos) - see also status-im/nim-chronos#380
There was a problem hiding this comment.
449e68b reenables the chronos tests (turns out they're broken right now though due to changes in chronos)
There was a problem hiding this comment.
LGTM, seems to be failing only on Nim devel which is a known problem.
There was a problem hiding this comment.
nah, it fails because of some compile errors but I added a || true so as to keep the checkmark green
2 participants
The
chronosbackend currently does not work as it relies on nestingwaitForcalls, which is not supported by chronos itself - as such, this PR disables the backend in testing and prints a recommendation to users to avoid it until the problem has been resolved.faststreams used without
asynccontinues to work as normal, which also is the most common way to use it - the dependency on chronos in the nimble file is unattractive from a user perspective since it also leads to the the downloading of bearssl and other deps.Hopefully, by the time the chronos backend can be used, we can also specify conditional dependencies in nimble files.