tracing
tracing copied to clipboard
tokio-rs
Bump matchers
Motivation
See #2945, this is an alternative to that.
Solution
Instead of removing it, make use of the recently released version.
(Not sure the tests pass as some tests fail on my Windows machine without this modification... So better to run it here.)
Edit: ~some obvious errors, I'll update when I believe more in it working...~ Seems to be OK now. Same error, but it has changed name between 0.1 and 0.2, so not clear if one can include this in the 0.3-series...
Same error, but it has changed name between 0.1 and 0.2, so not clear if one can include this in the 0.3-series...
From a quick glance, it looks like this error type is on the FromStr impl for MatchPattern and MatchPattern is not part of the public API. So this change should be okay.
You are correct. I thought parse_regex was part of the public API somehow, but was wrong.
Btw, I noted that you have a separate branch for the releases. Is this, if merged, (automatically) backported or should I create a similar PR against the v0.1.x-branch?
(I'm just the regex-automata author here looking to see folks move off of regex-automata 0.1, I'm not sure about tokio's processes here. Apologies.)
Same error, but it has changed name between 0.1 and 0.2, so not clear if one can include this in the 0.3-series...
From a quick glance, it looks like this error type is on the
FromStrimpl forMatchPatternandMatchPatternis not part of the public API. So this change should be okay.
Yup, that's correct, this shouldn't be a breaking change AFAICT.
Btw, I noted that you have a separate branch for the releases. Is this, if merged, (automatically) backported or should I create a similar PR against the v0.1.x-branch?
We merge PRs into the master branch and they are backported to v0.1.x by the maintainers. I can take care of that. Thank you!
Why not go with the 90 loc in https://github.com/tokio-rs/tracing/pull/2945 if this needs extra code anyways?
I think that, in order to fix that, we should add our own private, internal error type for the
FromStrimpl for MatchPatternthat wraps thematchers::BuildErrorand implementsstd::error::Error. This wrapper type doesn't need to be public, it just needs to be able to be converted into aBox<dyn std::error::Error + Send + Sync>`
I'll check with more rust-knowledgeable colleagues what this really means and see if I can implement that. :-)
Then, I guess, one can compare with #2945.
(It also seems to make sense, and possibly be easier, to have matchers::BuildError to implement std::error::Error? Although that requires an additional release of matchers etc. Anyway, I opened https://github.com/hawkw/matchers/issues/7 . So let's see what comes first...)
Edit: BuildError comes from regex_automata::dfa::dense and the following code exists:
https://github.com/rust-lang/regex/blob/8856fe36ac7dc37989e6ffb26b5fc57189bae626/regex-automata/src/meta/error.rs#L96-L104
It seems like tracing-subscriber imports regex with the std feature:
https://github.com/tokio-rs/tracing/blob/acf92ab944f6fa0c9dee0302a334d4fdf0cfe52a/tracing-subscriber/Cargo.toml#L46
which in turn should import regex_automata with the std feature:
https://github.com/rust-lang/regex/blob/8856fe36ac7dc37989e6ffb26b5fc57189bae626/Cargo.toml#L42-L47
If I read the blame correct, this was included in regex 1.9. I'll bump that dependency to see what happens and then one can discuss if that is a viable solution.
Alternatively, I think (my detailed knowledge of the Rust eco-system is not 100%) one can add a dependency on regex_automata with the std feature if one would like to avoid bumping regex?
@hawkw Would you mind starting the workflow? Interesting to see if this solves the issue and then we/you can discuss if it is an acceptable solution.
I'd also like to see the duplicate regex-automata version eliminated. As in, a variation of this PR backported to 0.1.x.
If I read the blame correct, [a dep on
regex-automata/std] was included in regex 1.9 [whenregex/stdis enabled]. I'll bump that dependency to see what happens and then one can discuss if that is a viable solution.
I think that works now but is fragile. regex-automata isn't part of regex's public API afaict, so some future version of regex could stop depending on regex-automata/std.
Alternatively, I think (my detailed knowledge of the Rust eco-system is not 100%) one can add a dependency on regex_automata with the std feature if one would like to avoid bumping regex?
I think that's better, or matchers always enabling regex-automata/std. I sent off a PR for the latter just now: https://github.com/hawkw/matchers/pull/8
ping. Is there any chance we can get this merged? It's still resulting in a lot of binary bloat downstream.
Closes #2923, ~I tried rebasing this PR and I am getting a bunch of tests failures (both with this PR and on master, might be a NixOS issue though)~ (I wasn't setting --test-threads=1, now it works).
ping @oscargus @hawkw any chance we could finish this PR soon? The downstream issues persist.
I was going to open PR for bumping matchers but checked the tracker out and found 4 opened PR's regarding this issue.
Any chances to see this soon? It is almost a year since it was opened.
Since matchers hasn't accepted to PR, I've updated this to add an explicit dependency on regex_automata. Not sure if this affects MSRV, but let's see...
Sorry for letting this slip, but it seemed like a better idea to add the dependency in matchers, so I thought that was going to happen.
Looks like this needs a rust-version bump from 1.63 to 1.65. I'm not a tracing maintainer but I would expect that to be fine. Could you update the PR? Might also need changes in .github/workflows.
Bumped to 1.65. As I changed the CI to run on 1.65, I also bumped all other subcrates to that. Not sure if that is the way to go or not, so split into two commits, where the second in the general bump (and, hence, easy to revert).
(No more changes today, but will be happy to fix any outstanding issues/adjust MSRV tomorrow.)