wooorm / markdown-rs

CommonMark compliant markdown parser in Rust with ASTs and extensions
https://docs.rs/markdown/1.0.0-alpha.21/markdown/
MIT License
906 stars 50 forks source link

build(cargo): allow to control reqwest tls backend #35

Closed kwonoj closed 1 year ago

kwonoj commented 1 year ago

This PR allows to control TLS backend for the dependency reqwest, while makes default (native-tls) as-is. Only if someone need to explicitly control they can disable default, then opt into specific.

reqwest is build-dependencies, so may seems it doesn't cause any problem: however, due to cargo creates lockfile & compiles it can actually brings some interesting challenges.

// Cargo.toml: Application

[dependencies]
LibA = *

// Cargo.toml: LibA
[dependencies]
reqwest = {.., features = ["rustls-tls"]}
markdown = *

// Cargo.toml: markdown

[build-dependencies]
reqwest = * // default, native-tls is enabled

Now building Application generates a lockfile for it, and cargo merges every feature for the dependencies into single like

reqwest = {
...
features = ["rustls-tls", "native-tls"]
}

regardless of type of dependencies. There are target platforms require specific TLS backend only, now building Application fails on those platform.

I tried to make this change non-invasive as much. Default works exactly same as current, and only explicit opt-in would change its behavior. Also added cases for 1. for disabling tls entirely, attempt to use http with warning 2. specifying both tls fails build.

wooorm commented 1 year ago

Did you see https://github.com/wooorm/markdown-rs/issues/34? I mistook what “build” means in Rust, so the plan is to move it to dev dependencies.

Would doing so that also solve this issue?

kwonoj commented 1 year ago

I mistook what “build” means in Rust, so the plan is to move it to dev dependencies.

As far as I know, if reqwest lives in any kind of dependencies, this change is still meaningful. That's the part I mentioned regardless of type of dependencies. If newly proposed change will get rid of reqwest entirely from cargo.toml, so upstream application's cargo.lock not being affected, it'll resolve the issue.

wooorm commented 1 year ago

From what I see dev-dependencies of dependencies are not in cargo.locks, so that should solve it?

kwonoj commented 1 year ago

If that's the case, yes, I think so. But I was not able to confirm all of the build chains by myself with those.

kwonoj commented 1 year ago

Just to clarify I'm fine either cases to move away dep from build, or trying this way - please feel free to close this one if needed. I also think moving dep away would be more preferred way.

wooorm commented 1 year ago

Fixed in https://github.com/wooorm/markdown-rs/releases/tag/1.0.0-alpha.5!