Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump matchers #3033

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bump matchers #3033

wants to merge 2 commits into from

Conversation

oscargus
Copy link

@oscargus oscargus commented Jul 14, 2024

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...

@oscargus oscargus requested review from hawkw, davidbarsky and a team as code owners July 14, 2024 20:23
@oscargus oscargus marked this pull request as draft July 14, 2024 20:26
@oscargus oscargus marked this pull request as ready for review July 14, 2024 20:31
@BurntSushi
Copy link

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.

@oscargus
Copy link
Author

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?

@BurntSushi
Copy link

(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.)

@mladedav
Copy link
Contributor

Generally, the maintainers handle backporting to v0.1.x.

@hawkw
Copy link
Member

hawkw commented Jul 21, 2024

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.

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!

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

It looks like the matchers::BuildError type doesn't implement std::error::Error, which breaks some code that uses ? on it in a method that returns Box<dyn Error + Send + Sync>: https://github.com/tokio-rs/tracing/actions/runs/9930533323/job/27719987902?pr=3033#step:7:218

I think that, in order to fix that, we should add our own private, internal error type for the FromStr impl 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 a Box<dyn std::error::Error + Send + Sync>`

@sophie-h
Copy link

sophie-h commented Aug 1, 2024

Why not go with the 90 loc in #2945 if this needs extra code anyways?

@oscargus
Copy link
Author

oscargus commented Aug 2, 2024

I think that, in order to fix that, we should add our own private, internal error type for the FromStr impl 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 a Box<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 hawkw/matchers#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:

regex = { optional = true, version = "1.6.0", default-features = false, features = ["std", "unicode-case", "unicode-perl"] }

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?

delan added a commit to delan/servo that referenced this pull request Aug 13, 2024
Signed-off-by: Delan Azabani <dazabani@igalia.com>
@oscargus
Copy link
Author

@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.

scottlamb added a commit to scottlamb/matchers that referenced this pull request Aug 23, 2024
Some usage in tracing expects this impl, as mentioned here:
<tokio-rs/tracing#3033 (review)>
It's provided when `regex-automata` has its `std` feature enabled.
As `matchers` uses `std` itself, no reason not to allow `regex-automata`
to do so also. Seems like the most straightforward way to address the
problem:

* avoids the need to create a wrapper type.
* less fragile than bumping `tracing-subscriber`'s `regex` dependency
  to 1.9.0 and hoping that all semver-compatible versions will continue
  depending on `regex-automata` with this `std` feature enabled.
@scottlamb
Copy link

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 [when regex/std is 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: hawkw/matchers#8

@asomers
Copy link
Contributor

asomers commented Dec 13, 2024

ping. Is there any chance we can get this merged? It's still resulting in a lot of binary bloat downstream.

@kanpov
Copy link

kanpov commented Dec 16, 2024

Another ping, seems like an easy change to make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants