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

remove log dependency #1666

Closed
wants to merge 3 commits into from
Closed

remove log dependency #1666

wants to merge 3 commits into from

Conversation

carllerche
Copy link
Member

Currently, Mio depends on log. In rust-lang/log#552, the log maintainers aim to increase log's MSRV to 1.60. This MSRV increase would impact Mio, which in turn, would impact Tokio. As of 1.25, Tokio supports Rust 1.49 with the intent of supporting it until March 2024.

Mio mostly uses the log dependency for debugging/tracing. Removing the log dependency has minimal impact and is the easiest way to resolve the MSRV issue. Additionally, this is another step towards minimizing Tokio's dependencies.

Currently, Mio depends on `log`. In rust-lang/log#552, the `log`
maintainers aim to increase `log`'s MSRV to 1.60. This MSRV increase
would impact Mio, which in turn, would impact Tokio. As of 1.25, Tokio
supports Rust 1.49 with the intent of supporting it until March 2024.

Mio mostly uses the `log` dependency for debugging/tracing. Removing the
`log` dependency has minimal impact and is the easiest way to resolve
the MSRV issue. Additionally, this is another step towards minimizing
Tokio's dependencies.
@carllerche
Copy link
Member Author

I have no idea why FreeBSD CI is failing.

@Thomasdezeeuw
Copy link
Collaborator

I'm very much against this. I use the logs to debug problems.

@carllerche
Copy link
Member Author

Are you OK with making it a dev dependency?

@carllerche
Copy link
Member Author

Though, I don't think there is a great way to check if dev dependencies are available conditionally.

@Thomasdezeeuw What do you propose? Tokio cannot depend either directly or transitively on any crates that break its MSRV policy.

@Thomasdezeeuw
Copy link
Collaborator

No as users need to enable it when the have a problem, if we have it in the tests only it's pretty useless.

@Thomasdezeeuw
Copy link
Collaborator

Thomasdezeeuw commented Apr 12, 2023

Though, I don't think there is a great way to check if dev dependencies are available conditionally.

@Thomasdezeeuw What do you propose? Tokio cannot depend either directly or transitively on any crates that break its MSRV policy.

That's nearly every crate. A MSRV policy just doesn't work, we need something better like e.g. 6 months support of released rust versions, but that's a large story.

@Darksonn
Copy link
Contributor

Another option is to gate it by a --cfg flag. Then, downstream users can enable it if they want.

@carllerche
Copy link
Member Author

That's nearly every crate. A MSRV policy just doesn't work, we need something better like e.g. 6 months support of released rust versions, but that's a large story.

Perhaps, but for a library like Tokio, I consider the ability to have toolchain stability valuable. We have an LTS/MSRV policy today that we need to try to maintain.

@KodrAus
Copy link

KodrAus commented Apr 12, 2023

It might make sense for mio to define its own diagnostics APIs that we can plug log or tracing or Big Corp's Bespoke Logging into.

That way we could minimize the amount of code mio itself depends on, and let you tailor those diagnostics very specifically to mio's needs.

@Thomasdezeeuw
Copy link
Collaborator

It might make sense for mio to define its own diagnostics APIs that we can plug log or tracing or Big Corp's Bespoke Logging into.

That way we could minimize the amount of code mio itself depends on, and let you tailor those diagnostics very specifically to mio's needs.

Isn't that the point of the log crate? It's only a facade and not a concrete implementation because of this right?

@KodrAus
Copy link

KodrAus commented Apr 12, 2023

@Thomasdezeeuw You're totally right, and we try hard to make it as broadly compatible and opinion-free as possible. But it's still an outside artifact. If you need strict control over some property of your system that a dependency isn't satisfying then you're pretty much stuck taking responsibility for implementing it yourself.

@Thomasdezeeuw
Copy link
Collaborator

I'm not sure if it will work but can Tokio (v1.25) add a direct dependency on log setting the version to =0.4.17, I think that will not allow upgrading beyond v0.4.17. It's an annoying limitation for people who don't care about the MSRV, but it might resolve the issue at least.

@carllerche
Copy link
Member Author

I don’t believe that would work. Given a dependency tree, finding the idea set of versions that satisfies it is an NP complete problem. To run dependency resolution in a reasonable time, cargo will opt to duplicate dependencies.

In this case, depending on how it traverses the dep tree, it could end up with two versions of log. Besides still having the MSRV issue, multiple versions of log could cause other problems for users because log uses static variables to store the specific logger.

@LukeMathWalker
Copy link

LukeMathWalker commented Apr 13, 2023

I might be misremembering, but I don't think cargo allows for multiple versions of the same crate in the dependency tree if they belong to the same "semver series" (e.g. you can't have 0.4.1 and 0.4.3 of the same crate in your dep tree, while having 0.4.3 and 0.3.0 is allowed).

@LukeMathWalker
Copy link

LukeMathWalker commented Apr 13, 2023

On the subject of MSRV, I think your interpretation of the MSRV policy for mio is excessively stringent here.
As long as mio doesn't require log >= 0.4.18 (which forces the use of Rust 1.60), it is possible to use the latest version of mio in a project built with Rust 1.49—it's the responsibility of the project owner to pin transitive dependencies to an acceptable range (until an MSRV-aware dependency resolver lands in cargo itself).

Removing log (or, even worse, forcing the entire ecosystem to hold back by pinning it) is unnecessary in my opinion.

@carllerche
Copy link
Member Author

Tokio maintains LTS branches to support users that prefer toolchain stability (i.e., they would instead update occasionally vs. constantly). In case of a security fix or a major bug fix, we will backport these fixes to the LTS branches. This way, more conservative users can get the fixes without the risk of disruption from regressions introduced by more significant changes.

In the event of a security or significant bug in a transitive dependency, we also need Tokio users to be able to get those fixes. For example, if a security issue is discovered and fixed in log, Tokio users on an LTS branch need to be able to get that patch. If log has since raised its MSRV, we are stuck.

Additionally, I would like Tokio's LTS branch CI to be green. When we backport fixes to LTS branches, I would like the process to be as smooth as possible. We don't currently check in Cargo.lock files in, and I would rather not have to do so in the future (for the same reason, it is not recommended for libraries to check in Cargo.lock files to git).

Removing log (or, even worse, forcing the entire ecosystem to hold back by pinning it) is unnecessary, in my opinion.

I agree that forcing the entire ecosystem to a pinned version of log would not be ideal. Given the points above, I see two ways forward. a) log sticks with a more conservative MSRV, or b) Tokio removes log from its dependency tree. I am not here to dictate what crates outside of tokio-rs do, so I'm currently leaning toward option b.

@hawkw
Copy link
Member

hawkw commented Apr 13, 2023

A potential alternative solution would be to replace the log dependency in mio with tracing, allowing us to continue emitting the same diagnostics. tracing is part of the tokio-rs org, and its MSRV will always be compatible with tokio's MSRV (I'm willing to commit to this guarantee as a maintainer). tracing has an optional, default-off log dependency, which upstream mio users can still enable if they want to collect log diagnostics, but it does not include non-MSRV-compliant crates in its dependency tree by default.

Mio's tests etc can probably be converted to use tracing with relatively little effort.

@LukeMathWalker
Copy link

LukeMathWalker commented Apr 13, 2023

In the event of a security or significant bug in a transitive dependency, we also need Tokio users to be able to get those fixes. For example, if a security issue is discovered and fixed in log, Tokio users on an LTS branch need to be able to get that patch. If log has since raised its MSRV, we are stuck.

Having log behind a feature flag seems to be enough to handle this scenario (at least in the immediate remediation phase)?

@Thomasdezeeuw
Copy link
Collaborator

A potential alternative solution would be to replace the log dependency in mio with tracing, allowing us to continue emitting the same diagnostics. tracing is part of the tokio-rs org, and its MSRV will always be compatible with tokio's MSRV (I'm willing to commit to this guarantee as a maintainer). tracing has an optional, default-off log dependency, which upstream mio users can still enable if they want to collect log diagnostics, but it does not include non-MSRV-compliant crates in its dependency tree by default.

I don't really want to switch to the tracing crate as the log crate is sufficient for our purposes, furthermore that would be a breaking change as the logs won't appear if you don't have a tracing implementation/logger set up (assuming they don't enable the log feature on the tracing crate).

I agree that forcing the entire ecosystem to a pinned version of log would not be ideal. Given the points above, I see two ways forward. a) log sticks with a more conservative MSRV, or b) Tokio removes log from its dependency tree. I am not here to dictate what crates outside of tokio-rs do, so I'm currently leaning toward option b.

I don't think it's fair to the ecosystem to hold back the log crate because some users require support for a very old rust version, so I'd say a is unreasonable.

That leaves option b... but I don't want to remove it for non-Tokio TLS users as the logs allow you to quickly debug issues.

Maybe Tokio TLS version can use mio-tls version that uses an older MSRV that removes the log dependency. The problem there is that if people report a problem with this version they'll need to do some kind of logging or system call tracing on their own.

@carllerche
Copy link
Member Author

Maybe Tokio TLS version can use mio-tls version that uses an older MSRV that removes the log dependency. The problem there is that if people report a problem with this version they'll need to do some kind of logging or system call tracing on their own.

Do you mean maintaining a fork of mio named mio-lts that removes the log dependency?

When it comes to debugging problems, could we rely on tools like strace and other OS level tooling for getting data? What sort of data do you hope to get on top of what OS-level tooling can provide?

@Keruspe
Copy link
Contributor

Keruspe commented Apr 14, 2023

@Thomasdezeeuw how hard would it be to add a feature to use tracing instead of log?
By default mio would still use and depend on log, and if tracing is enabled, it would use and depend on that instead.

@NobodyXu
Copy link

@Thomasdezeeuw how hard would it be to add a feature to use tracing instead of log?
By default mio would still use and depend on log, and if tracing is enabled, it would use and depend on that instead.

tracing has a feature "log" which brings in log as a dependency and emit log events instead.

So maybe mio could actually switch to tracing, add a new feature "log" that is enabled by default for backwards compatibility.

Crates like tokio can disable default features so that it is not affected by the msrv bump.

@Thomasdezeeuw
Copy link
Collaborator

Maybe Tokio TLS version can use mio-tls version that uses an older MSRV that removes the log dependency. The problem there is that if people report a problem with this version they'll need to do some kind of logging or system call tracing on their own.

Do you mean maintaining a fork of mio named mio-lts that removes the log dependency?

@carllerche Yes, just another branch in this repo that Tokio TLS versions can depend on.

When it comes to debugging problems, could we rely on tools like strace and other OS level tooling for getting data? What sort of data do you hope to get on top of what OS-level tooling can provide?

Yes, strace would be sufficient, however that's harder to run (in production) that enabling tracing logging for most users.

@Keruspe @NobodyXu I have zero interest in switch to tracing. tracing is a heavier dependency than log and not all users of Mio use Tokio. Forcing all users of Mio to switch to tracing not desired.

@carllerche
Copy link
Member Author

Yes, just another branch in this repo that Tokio TLS versions can depend on.

If we go that route, I would most likely lean towards pulling Mio into Tokio itself instead of adding a separate crate.

@Thomasdezeeuw
Copy link
Collaborator

If we go that route, I would most likely lean towards pulling Mio into Tokio itself instead of adding a separate crate.

Well it won't be a separate it's just Mio, slightly modified for old Tokio versions. But if you prefer to maintain Mio inside of Tokio itself, that's always an option.

@Darksonn
Copy link
Contributor

Has the possibility of having it be an ordinary optional dependency been discussed anywhere?

@KodrAus
Copy link

KodrAus commented Apr 18, 2023

Has the possibility of having it be an ordinary optional dependency been discussed anywhere?

Is the concern there just that applications that are already deployed would need to be rebuilt in order to get diagnostics when they misbehave? A feature gate seems like the path-of-least-surprise to me though.

@Thomasdezeeuw
Copy link
Collaborator

Has the possibility of having it be an ordinary optional dependency been discussed anywhere?

Not yet.

Has the possibility of having it be an ordinary optional dependency been discussed anywhere?

Is the concern there just that applications that are already deployed would need to be rebuilt in order to get diagnostics when they misbehave? A feature gate seems like the path-of-least-surprise to me though.

We could make the log dependency optional and enable it by default. Then Tokio versions that support MSRV < 1.60 can disable it.

@Thomasdezeeuw
Copy link
Collaborator

@carllerche what do you think of @Darksonn's idea?

We could make the log dependency optional and enable it by default. Then Tokio versions that support MSRV < 1.60 can disable it.

@carllerche
Copy link
Member Author

Closed in favor of #1673

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