Deprecate / remove driver in favour of binder

[Sytten]

I don't think the two should be maintained as it is confusing for the users and the binder is a better / more flexible solution long term.

[tyt2y3]

Umm yes. But driver also works for other DB drivers, i.e. the vanilla postres and rusqlite. So we either have to migrate (I think postgres should be easy because we have been implementing ToSql all the time) or maintain both. We can remove SQLx support from driver as the first step though.

[ikrivosheev]

@tyt2y3 hello! I have a PoC for this. I can draft PR.

[tyt2y3]

@tyt2y3 hello! I have a PoC for this. I can draft PR.

Sure

[ikrivosheev]

@tyt2y3 maybe it is better to create another crate for rusqlite and Postgres?

[tyt2y3]

Um... not sure will it be a lot of hassle if we use feature flags. Otherwise, if we have multiple crates, isn't it similar to the situation we are in now (having multiple driver crates) ?

[ikrivosheev]

@Sytten @tyt2y3 I create PR for rusqlite: https://github.com/SeaQL/sea-query/pull/388. I have a problem. I cannot create it in sea-query-binder, because I got error:

error: failed to select a version for `libsqlite3-sys`.
    ... required by package `sqlx-core v0.6.0`
    ... which satisfies dependency `sqlx-core = "^0.6.0"` of package `sqlx v0.6.0`
    ... which satisfies dependency `sqlx = "^0.6"` of package `sea-query-binder v0.1.0 (/home/ikrivosheev/projects/opensource/sea-query/sea-query-binder)`
    ... which satisfies path dependency `sea-query-binder` (locked to 0.1.0) of package `sea-query-sqlx-any-example v0.1.0 (/home/ikrivosheev/projects/opensource/sea-query/examples/sqlx_any)`
versions that meet the requirements `^0.24.1` are: 0.24.2, 0.24.1

the package `libsqlite3-sys` links to the native library `sqlite3`, but it conflicts with a previous package which links to `sqlite3` as well:
package `libsqlite3-sys v0.25.0`
    ... which satisfies dependency `libsqlite3-sys = "^0.25.0"` of package `rusqlite v0.28.0`
    ... which satisfies dependency `rusqlite = "^0.28"` of package `sea-query-rusqlite v0.1.0 (/home/ikrivosheev/projects/opensource/sea-query/sea-query-rusqlite)`
    ... which satisfies path dependency `sea-query-rusqlite` of package `sea-query-rusqlite-example v0.1.0 (/home/ikrivosheev/projects/opensource/sea-query/examples/rusqlite)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the links ='libsqlite3-sys' value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

failed to select a version for `libsqlite3-sys` which could resolve this conflict

Maybe do you know work around?

[Sytten]

Usually one solution to that is to have one crate per integration like sea-query-binder-sqlx and sea-query-binder-rustqlite. The other solution is to make sure only one integration is enabled at a time with optional dependencies but that only works if you dont yourself need the transitive dependency.

I think the one crate per integration is the better approach. I would also consider naming them sea-query-sqlx directly.

[ikrivosheev]

@Sytten I made another crate for rusqlite. You can see my PR, but it doesn't help. Now cargo thinks that have two roots in one workspace

[Sytten]

Hum lets just add it to the root workspace? In theory it should not conflict with the sqlite version of sqlx since its a different crate.

[ikrivosheev]

@Sytten no... Because Cargo.lock is one for workspace.

[tyt2y3]

Ah I think Rusqlite and SQLx has a conflicting requirement to libsqlite3 The only solution is to not put the crate in the global workspace. I mean adding the following to the Cargo.toml

[workspace]

I think we have a similar setup in SeaORM

[Sytten]

Yeah I linked the issue in cargo for that. It is a bit annoying but no other choice.

[ikrivosheev]

Ah I think Rusqlite and SQLx has a conflicting requirement to libsqlite3 The only solution is to not put the crate in the global workspace. I mean adding the following to the Cargo.toml

[workspace]

I think we have a similar setup in SeaORM

If I do this, I got error:

cargo build
error: multiple workspace roots found in the same workspace:
  /home/ikrivosheev/projects/opensource/seaql/sea-query
  /home/ikrivosheev/projects/opensource/seaql/sea-query/sea-query-rusqlite

[ikrivosheev]

@Sytten @tyt2y3 @billy1624 I created PRs.

• https://github.com/SeaQL/sea-orm/pull/970 - remove sea-query-driver from SeaORM
• https://github.com/SeaQL/sea-query/pull/423 - rewrite examples using sea-query-binder
• https://github.com/SeaQL/sea-query/pull/416 - rewrite CI and add simple features to sea-query-binder
• https://github.com/SeaQL/sea-query/pull/422 - create new crate for Rusqlite

After merge these PRs we can deprecated/remove sea-query-driver.

[billy1624]

Hey @ikrivosheev, about the updated CI. I think...

1. binder-linter could be merged with linter
2. binder-build should always run regardless of binder-linter
3. Same for build, it should be run regardless of linter

I think clippy and fmt is just a additional checking but not prerequisites.

REF: https://github.com/SeaQL/sea-query/pull/423/commits/a0575638f3619277c3cce454f37e217b58da89e2

[billy1624]

Also, I notice the updated CI spawn a instance to compile the crate with a specific set of features enabled. I guess the old way (single instance multiple steps) might compile faster. Because it only compile the changes but not the whole library every single time.

The old way:

• https://github.com/SeaQL/sea-query/blob/6dd6ae8eb216f3a5e72653940f0b1decc0f4db7c/.github/workflows/rust.yml#L38-L83

[billy1624]

The merge sequence would be?

1. https://github.com/SeaQL/sea-query/pull/416 - rewrite CI and add simple features to sea-query-binder
2. https://github.com/SeaQL/sea-query/pull/423 - rewrite examples using sea-query-binder
3. https://github.com/SeaQL/sea-query/pull/422 - create new crate for Rusqlite
4. https://github.com/SeaQL/sea-orm/pull/970 - remove sea-query-driver from SeaORM

@ikrivosheev feel free to rearrange the list.

[tyt2y3]

Thanks! Sounds like a good plan

[ikrivosheev]

The merge sequence would be?

1. [issues-383 Improve sea-query-binder #416](https://github.com/SeaQL/sea-query/pull/416) - rewrite CI and add simple features to `sea-query-binder`

2. [ issues-383 Rewrite examples using sea-query-binder #423](https://github.com/SeaQL/sea-query/pull/423) - rewrite examples using `sea-query-binder`

3. [ issues-383 Create new crate for rusqlite #422](https://github.com/SeaQL/sea-query/pull/422) - create new crate for Rusqlite

4. [issues-969 Replace sea-query-driver to sea-query-binder sea-orm#970](https://github.com/SeaQL/sea-orm/pull/970) - remove `sea-query-driver` from SeaORM

@ikrivosheev feel free to rearrange the list.

Yes, this is the correct order.

[ikrivosheev]

@billy1624 I have corrected all comments about CI in all PRs.

[billy1624]

Hey @ikrivosheev, the CI looks good :) Thanks!!

[Sytten]

Down the line I am not sure binders will be necessary at all if we switch the way value are handled and we use optional features. I guess its not a bad step in the meantime.

[ikrivosheev]

Down the line I am not sure binders will be necessary at all if we switch the way value are handled and we use optional features. I guess its not a bad step in the meantime.

I think we need some steps before doing this...

[ikrivosheev]

I reopen the issues, PR:

1. https://github.com/SeaQL/sea-query/pull/422
2. https://github.com/SeaQL/sea-query/pull/423
3. https://github.com/SeaQL/sea-orm/pull/970

[ikrivosheev]

New PR: https://github.com/SeaQL/sea-query/pull/431. I forget about thread-safe

[tyt2y3]

Yeah if you put resolves #XX in the PR body it will close the issue automatically

[tyt2y3]

Actually, I think it also makes sense (to be architecturally consistent) to separate postgres support to a dedicated crate as well:

https://github.com/SeaQL/sea-query/blob/70120b7c99eb40009782066fbb9496d9638c2696/Cargo.toml#L35

That would make sea-query a (more or less) 'pure' library.

[ikrivosheev]

Actually, I think it also makes sense (to be architecturally consistent) to separate postgres support to a dedicated crate as well:

https://github.com/SeaQL/sea-query/blob/70120b7c99eb40009782066fbb9496d9638c2696/Cargo.toml#L35

That would make sea-query a (more or less) 'pure' library.

@tyt2y3 hello! I create PR with this feature: https://github.com/SeaQL/sea-query/pull/433

[ikrivosheev]

@tyt2y3 when we can remove sea-query-driver from source code?

[tyt2y3]

I added a deprecate notice to sea-query-driver, so I think it's good to remove now!