servo / rust-url

URL parser for Rust
https://docs.rs/url/
Apache License 2.0
1.33k stars 331 forks source link

Compilation error with url 2.5.3 / idna 1.0.3 with Rust < 1.81 and edition 2021 (resolver 2) when sqlx-core is in the dependency graph #992

Open TotalKrill opened 3 weeks ago

TotalKrill commented 3 weeks ago

Edited by @hsivonen to put resolution options at the top for folks who come here:

This compilation error is due to a combination of:

  1. SQLx unnecessarily turning off the default features of url, and from a proc macro context specifically. (Request for SQLx not to do that: https://github.com/launchbadge/sqlx/issues/3589)
  2. url 2.5.3 (and idna 1.0.3) conditionally raising MSRV to 1.81 when default features are off. (From #831)
  3. resolver = "2", which is implied by edition = "2021" performing feature resolution for proc macro dependencies separately from normal dependencies, so a normal dependency on url with default features enabled does not cause default features to be enabled for the proc macro case.

The most obvious way to resolve this compilation error is to upgrade Rust to 1.81 or later. (If you are using SQLx 0.8.x, your Rust version has to already be 1.79 or later, so it's not a big jump.)

Alternative resolution without upgrading Rust:

  1. Put resolver = "1" in the [workspace] section of your top-level Cargo.toml. If there's no such section yet, create a [workspace] section at the top of your top-level Cargo.toml.
  2. If you don't already have a non-proc macro dependency on url with default features enabled somewhere in your dependency graph, add url = "2.5.3" to the [dependencies] section of one of your crates (the top-level is the most obvious place, but it does not have to be on the top level).

(Conditionally raising MSRV to 1.81 is not a semver break, because raising MSRV isn't a semver break.)

Original report follows:

Describe the bug

After update to v1.0.3 wich is picked up automatically since its a patch upgrade, our code no longer compiles the same.

rust versions tested:

   Compiling idna v1.0.3
error[E0658]: use of unstable library feature 'error_in_core'
  --> /home/krille/.cargo/registry/src/index.crates.io-6f17d22bba15001f/idna-1.0.3/src/lib.rs:78:6
   |
78 | impl core::error::Error for Errors {}
   |      ^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #103765 <https://github.com/rust-lang/rust/issues/103765> for more information
valenting commented 3 weeks ago

@TotalKrill Is your crate depending on idna directly with default-features = false. Or is the issue url failing to propagate the std feature to idna?

TotalKrill commented 3 weeks ago

We are depending on url, not idna directly

TotalKrill commented 3 weeks ago

@valenting seems i was wrong about 1.81.0, seems to work there...

hsivonen commented 3 weeks ago

Are you depending on url with default-features = false?

I tried to repro on 1.80.1, and the following compiles:

rustup default 1.80
cargo new --bin testurl
cd testurl
cargo add url
cargo build
TotalKrill commented 3 weeks ago

we have a workspace, but we only depend on url like this: url = "2.5.0"

TotalKrill commented 3 weeks ago

running cargo tree -i url -f "{p} {f}"

this is what our tree ends up including, with two different versions of URL included, probably by one of our other deps.

Edit: I meant version 2.5.3, 2.5.0 is the version that compiles

hsivonen commented 3 weeks ago

url 2.5.0 and url 2.5.2 depended on idna 0.5. url 2.5.1 depended on idna 1.0.

The introduction of no_std support to url in 2.5.3 and the introduction of core::error::Error to idna in 1.0.3 was intended not to change MSRV to 1.81 with default features but was expected to change MSRV to 1.81 for default-features = false users.

Do you somehow have url 2.5.1 in your dependency graph? Can you force Cargo to take url 2.5.3 in its place?

TotalKrill commented 3 weeks ago

Oh sorry, I am currently forcing our software to take 2.5.0, since 2.5.3 does not compile. Updated the previous comment to show that the cargo tree command showed 2.5.3

hsivonen commented 3 weeks ago

So if you have url 2.5.3 without features and url 2.5.3 with the feature std, Cargo should unify those to one copy of url with the feature std. Are you sure the edited comment about cargo tree is correct?

Aside: This isn't specific to idna. If I use Rust 1.80.1 with:

[dependencies]
url = { version = "2.5.3", default-features = false }
idna = "1"

...idna compiles and the compilation of url fails with a complaint about impl core::error::Error for ParseError {} in url.

TotalKrill commented 3 weeks ago

yep, my edited comment stands. But with what you say, something else might be changing to default-features = false

hsivonen commented 3 weeks ago

As long as something in your dependency graph depends on url without setting default-features = false, something else setting default-features = false shouldn't matter.

@Twixes, GitHub autolinks a couple of your changesets referencing this issue. Do you have minimized steps to reproduce?

valenting commented 3 weeks ago

I think cargo tree -e features might explain if there's such a dependency conflict.

mental32 commented 3 weeks ago

Just felt this through my direct dependency to sqlx. I'm commenting here but I'm not sure who, sqlx or idna, is responsible in this case. (@hsivonen would appreciate your take here)

minimal verifiable example:

cd /tmp
cargo new idna-992-repro
cd idna-992-repro/
cargo add sqlx
cargo check

And I get the same compilation error as @TotalKrill

The latest version of SQLx is v0.8.2 in the example, but in my original encounter its v0.7.4.

(for the benefit of solution-seekers pinning to url = "=2.5.2" is the fix i am using for now.)

cargo --version --verbose ``` cargo 1.80.1 (376290515 2024-07-16) release: 1.80.1 commit-hash: 37629051518c3df9ac2c1744589362a02ecafa99 commit-date: 2024-07-16 host: aarch64-apple-darwin libgit2: 1.7.2 (sys:0.18.3 vendored) libcurl: 8.7.1 (sys:0.4.72+curl-8.6.0 system ssl:(SecureTransport) LibreSSL/3.3.6) ssl: OpenSSL 1.1.1w 11 Sep 2023 os: Mac OS 15.0.1 [64-bit] ```
rustc --version --verbose ``` rustc 1.80.1 (3f5fd8dd4 2024-08-06) binary: rustc commit-hash: 3f5fd8dd41153bc5fdca9427e9e05be2c767ba23 commit-date: 2024-08-06 host: aarch64-apple-darwin release: 1.80.1 LLVM version: 18.1.7 ```
hsivonen commented 3 weeks ago

As long as something in your dependency graph depends on url without setting default-features = false, something else setting default-features = false shouldn't matter.

Interestingly, this turns out not to be true.

rustup default 1.80
cargo new idna-992-repro
cd idna-992-repro/
cargo add sqlx
cargo add url
cargo check

Still fails. (Note the added cargo add url.)

@domenukk why does sqlx-core's dependency on url with default-features = false not unify with the top-level's dependency on url (with default features)?

TotalKrill commented 3 weeks ago

We are also using sqlx, so might be through that

hsivonen commented 3 weeks ago

sqlx-core has url = { version = "2.2.2", default-features = false }. How has that not failed due to url requiring alloc before?

Twixes commented 3 weeks ago

@hsivonen I pinned idna to 0.5 in Cargo.toml's [dependencies] – seems to solve the issue. It also appears to be stemming from sqlx for us

hsivonen commented 3 weeks ago

@hsivonen I pinned idna to 0.5 in Cargo.toml's [dependencies] – seems to solve the issue.

Do you mean https://github.com/PostHog/posthog/commit/5f6e5d1100e42cf099ec881fa6203e13fc0bc2f6 fixed it for you? That's really weird. By what mechanism should that help?

Twixes commented 3 weeks ago

Actually you are right, the Cargo.toml change doesn't help. It's the --locked arg of cargo install that's the fix.

For us, the issue was with this command: cargo install sqlx-cli@0.7.3 --no-default-features --features native-tls,postgres Used to work with Rust 1.77, but starting today it only works on 1.81+ – because of idna 1.0 getting installed. Our lockfile has already been specifying idna 0.5 though, now that I look at it.

By appending --locked to cargo install sqlx-cli@0.7.3 --no-default-features --features native-tls,postgres we make it so that the lockfile is respected, fixing installation. (It looks like there's no alternative way to restrict transitive dependencies in cargo install)

hsivonen commented 3 weeks ago

The sqlx change that added default-features = false was https://github.com/launchbadge/sqlx/commit/d76b1357da21f777ee1cdab8bfd907ed66c7b3bd which doesn't explain why.

hsivonen commented 3 weeks ago

Filed an sqlx issue: https://github.com/launchbadge/sqlx/issues/3589

domenukk commented 3 weeks ago

As long as something in your dependency graph depends on url without setting default-features = false, something else setting default-features = false shouldn't matter.

Interestingly, this turns out not to be true.

rustup default 1.80
cargo new idna-992-repro
cd idna-992-repro/
cargo add sqlx
cargo add url
cargo check

Still fails. (Note the added cargo add url.)

@domenukk why does sqlx-core's dependency on url with default-features = false not unify with the top-level's dependency on url (with default features)?

Have you figured that one out? I feel like it should? Different versions of the url crate?

hsivonen commented 3 weeks ago

As long as something in your dependency graph depends on url without setting default-features = false, something else setting default-features = false shouldn't matter.

Interestingly, this turns out not to be true.

rustup default 1.80
cargo new idna-992-repro
cd idna-992-repro/
cargo add sqlx
cargo add url
cargo check

Still fails. (Note the added cargo add url.) @domenukk why does sqlx-core's dependency on url with default-features = false not unify with the top-level's dependency on url (with default features)?

Have you figured that one out?

I have not.

I feel like it should?

I agree. Could this be a Cargo bug? The unusual thing that I notice is that the sqlx-* dependencies within the sqlx suite of crates are exact version dependencies. However, when sqlx-core and sqlx-macros-core depend on url 2.2.2 (not an exact-version dependency) with default-features = false, the crate does get upgraded to 2.5.3 by Cargo.

Can an exact version dependency up the graph from url confuse feature unioning in Cargo even when it does not confuse version upgradeability?

Different versions of the url crate?

No, Cargo.lock shows only one instance of url 2.5.3.

hsivonen commented 3 weeks ago

Posted a request for help with figuring out why Cargo is doing what it does: https://users.rust-lang.org/t/why-doesnt-cargo-compile-with-a-union-of-features/120716

abonander commented 3 weeks ago

Not really sure why we had default-features = false to begin with on a crate that had no feature flags, but strictly speaking we're being good citizens by doing this.

Because of Cargo's feature unification, it becomes very difficult to turn off default features for a crate that's deep in your dependency tree, since any other crate in the tree that depends on it but neglects to set default-features = false forces on the default features for the whole dependency tree. It's impossible to fix this without patching or overriding that dependency. We've run into this many times with other dependencies.

I think one of the last times we ran into this we just added default-features = false to a bunch of dependencies. You can see that's exactly what we did in the previously linked commit: https://github.com/launchbadge/sqlx/commit/d76b1357da21f777ee1cdab8bfd907ed66c7b3bd

It's a really weird situation that disabling a feature leads to an increase in the MSRV. I would consider this an unintentional breakage of the url crate, unless I missed the meeting where it was decided that setting default-features = false makes a dependency completely exempt from SemVer.

I understand that as a more "fundamental" crate than SQLx, url has different priorities, but issues like this are exactly why we decided not to guarantee an MSRV in the first place, because it's a massive SemVer hazard and it's really easy to make mistakes like this. Our current MSRV policy is a compromise between this sentiment and people not wanting random breakages in patch upgrades.

Manishearth commented 3 weeks ago

but strictly speaking we're being good citizens by doing this.

I see where you're coming from, but I also slightly disagree: I think it's pretty much a norm for crates to assume that splitting out code into a feature but including it in default is not a breaking change, so in this case you're open to breakages.

It's a tradeoff. There have been proposals for being able to say which version your feature spec is based on.

Manishearth commented 3 weeks ago

It's a really weird situation that disabling a feature leads to an increase in the MSRV

I also feel like this at least used to be somewhat expectable back in the days when no_std was not as polished. But I haven't seen that in ages.

hsivonen commented 3 weeks ago

(I edited the issue description to put solutions at the top of the page at this point in the thread.)

LiamPClancy commented 2 weeks ago

Not sure if you need any more data but just wanted to add what i'm using to resolve the issue - as it just cameup today, and my fix seems to have resolved it for the time being. I just wanted to comment here because I ran into the same issue, and not sure if it's resolved,

the error that i was getting trying to run this

cargo lambda build --release --arm64

when using sqlx migrations,

was

error[E0658]: use of unstable library feature 'error_in_core' --> /Users/liamclancy/.cargo/registry/src/index.crates.io-6f17d22bba15001f/idna-1.0.3/src/lib.rs:78:6

I have resolved this locally by adding this to my dependencies.

idna = "=1.0.2"

the issue only started happening today, so I assume it was the update to idna in the last week.