Smol+async global executor 1.80 dev

[martin-kolarik]

async-global-executor added as the fourth executor. The PR extends PR #3790.

This is a breaking change -- async-global-executor has MSRV 1.80, therefore sqlx will have to be upgraded too.

[abonander]

I would prefer to deprecate async-std for a release cycle before deleting it. Just cause it's discontinued doesn't mean people will stop using it immediately, especially because its constituent crates will continue to receive updates.

[martin-kolarik]

I would prefer to deprecate async-std for a release cycle before deleting it. Just cause it's discontinued doesn't mean people will stop using it immediately, especially because its constituent crates will continue to receive updates.

Sure, I did not remove async_std from the sources, for the same reason.

[abonander]

@martin-kolarik do you plan on addressing the CI failure and merge conflicts?

[martin-kolarik]

@martin-kolarik do you plan on addressing the CI failure and merge conflicts?

I would like to address both. I will look at merge conflict first (today or on Monday) and we will see how tests changed. CI failure is linked to changes around to_socket_addrs and I suspect tests fail due to changed behavior. I will write more later.

[martin-kolarik]

So, merged, fixed, and tests are running: the main issue of previous failures disappeared by itself, there certificate for localhost was expected.

CI completion is prevented with error in build using minimal versions check, where async-fs fails. According to https://github.com/rust-lang/cargo/issues/5657 it may happen. Did you have similar issues in the past? How to fix it?

[abonander]

Strictly speaking, this is a bug in async-fs 2.0.0 because they're calling futures_lite::FutureExt::poll() which did not exist yet in the version they actually specify (1.2).

The whole idea of the minimal-versions test is to make sure that SQLx still builds if someone hasn't updated some arbitrary dependency below it in the tree, so the test is working as expected here. However, this isn't strictly our problem to begin with.

We could switch to -Z minimal-direct-versions which only lowers our direct dependencies to their minimum specified versions, so we'd only be testing our own specifications. This gives limits the scope to dependencies we actually control, but also weakens the guarantees of the minimal-versions check (since indirect dependencies could still be technically broken).

I'd really like feedback from @iamjpotts here since he's the one who recommended adding the check to begin with (#3340).

[iamjpotts]

The whole idea of the minimal-versions test is to make sure that SQLx still builds if someone hasn't updated some arbitrary dependency below it in the tree, so the test is working as expected here. However, this isn't strictly our problem to begin with.

Yes, it is intended to be a correctness check on the declared minimums.

From that standpoint the ci check is working as designed. The original issue was mentioned in https://github.com/launchbadge/sqlx/issues/3118 (linked from https://github.com/launchbadge/sqlx/pull/3340).

async-fs fixed their dependency declaration in async-fs 2.1.

smol requires futures-lite 2.0, but in sqlx is optional.

The fix is likely to be adding an explicit dependency on a transitive dependency, where the explicit dependency enforces the correct minimum. Possible workarounds to explore:

• Add a sqlx dependency on async-fs 2.1
• Add a sqlx dependency on futures-lite 2.0

For either it would be a good idea to add a comment that the above is to resolve a problem with a transitive dependency (and which one, idk if it is smol without further investigation)

[martin-kolarik]

• Add a sqlx dependency on async-fs 2.1
• Add a sqlx dependency on futures-lite 2.0

I added dependency on async-fs 2.1, as it imo better describes the intention of the workaround: the async-fs 2.0 contains bug, not futures-lite.

[iamjpotts]

• Add a sqlx dependency on async-fs 2.1
• Add a sqlx dependency on futures-lite 2.0

I added dependency on async-fs 2.1, as it imo better describes the intention of the workaround: the async-fs 2.0 contains bug, not futures-lite.

This specific change lgtm though @abonander may have more fine grained feedback.

[abonander]

I'm going to merge to avoid the need for any more back-and-forth, but I'm going to be making some adjustments in a follow-up PR.

[martin-kolarik]

I'm going to merge to avoid the need for any more back-and-forth, but I'm going to be making some adjustments in a follow-up PR.

Thank you, unfortunately I was off during weekend so I did not respond to breaking change note. Feel free to ask for anything to be completed.