Deprecate / remove driver in favour of binder #383

Sytten · 2022-07-15T21:50:42Z

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 · 2022-07-17T08:36:50Z

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 · 2022-07-17T09:54:23Z

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

tyt2y3 · 2022-07-17T09:57:04Z

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

Sure

ikrivosheev · 2022-07-17T10:03:01Z

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

tyt2y3 · 2022-07-17T11:25:34Z

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 · 2022-07-17T13:08:44Z

@Sytten @tyt2y3 I create PR for rusqlite: #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 · 2022-07-17T16:08:28Z

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 · 2022-07-17T16:40:10Z

@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 · 2022-07-17T16:42:44Z

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 · 2022-07-17T17:42:48Z

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

tyt2y3 · 2022-07-23T06:00:55Z

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 · 2022-07-23T11:03:29Z

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

ikrivosheev · 2022-07-25T11:23:13Z

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 · 2022-08-20T11:27:28Z

@Sytten @tyt2y3 @billy1624 I created PRs.

issues-969 Replace sea-query-driver to sea-query-binder sea-orm#970 - remove sea-query-driver from SeaORM
issues-383 Rewrite examples using sea-query-binder #423 - rewrite examples using sea-query-binder
issues-383 Improve sea-query-binder #416 - rewrite CI and add simple features to sea-query-binder
issues-383 Create new crate for rusqlite #422 - create new crate for Rusqlite

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

billy1624 · 2022-08-23T10:20:54Z

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

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

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

REF: a057563

billy1624 · 2022-08-23T10:30:15Z

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:

sea-query/.github/workflows/rust.yml

Lines 38 to 83 in 6dd6ae8

    
             build: 
        
               name: Build 
        
               runs-on: ubuntu-20.04 
        
               steps: 
        
                 - uses: actions/checkout@v2 
        
                 - uses: actions-rs/toolchain@v1 
        
                   with: 
        
                     profile: minimal 
        
                     toolchain: stable 
        
                     override: true 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --no-default-features 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --all-features 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --features=thread-safe 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --features=with-chrono 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --features=with-json 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --features=with-rust_decimal 
        
                 - uses: actions-rs/cargo@v1 
        
                   with: 
        
                     command: build 
        
                     args: --features=with-uuid

billy1624 · 2022-08-23T11:04:23Z

The merge sequence would be?

issues-383 Improve sea-query-binder #416 - rewrite CI and add simple features to sea-query-binder
issues-383 Rewrite examples using sea-query-binder #423 - rewrite examples using sea-query-binder
issues-383 Create new crate for rusqlite #422 - create new crate for Rusqlite
issues-969 Replace sea-query-driver to sea-query-binder sea-orm#970 - remove sea-query-driver from SeaORM

@ikrivosheev feel free to rearrange the list.

tyt2y3 · 2022-08-24T14:42:33Z

Thanks! Sounds like a good plan

ikrivosheev · 2022-08-25T10:25:32Z

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 · 2022-08-25T10:36:18Z

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

billy1624 · 2022-08-26T09:08:59Z

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

Sytten · 2022-08-26T13:18:15Z

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 · 2022-08-26T13:31:44Z

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 · 2022-09-03T15:40:57Z

I reopen the issues, PR:

ikrivosheev · 2022-09-03T16:09:27Z

New PR: #431. I forget about thread-safe

tyt2y3 · 2022-09-04T02:08:06Z

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

tyt2y3 · 2022-09-04T06:07:02Z

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

sea-query/Cargo.toml

Line 35 in 70120b7

postgres-types = { version = "^0", optional = true }

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

ikrivosheev · 2022-09-04T12:37:36Z

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

sea-query/Cargo.toml

Line 35 in 70120b7

postgres-types = { version = "^0", optional = true }

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

@tyt2y3 hello! I create PR with this feature: #433

ikrivosheev · 2022-09-06T16:30:58Z

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

tyt2y3 · 2022-09-06T16:45:35Z

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

ikrivosheev mentioned this issue Jul 17, 2022

issues-383 Create new crate for rusqlite #388

Closed

This was referenced Aug 12, 2022

Support sea-query-binder SeaQL/sea-orm#969

Closed

issues-383 Improve sea-query-binder #416

Merged

billy1624 linked a pull request Aug 19, 2022 that will close this issue

issues-383 Rewrite examples using sea-query-binder #387

Closed

This was referenced Aug 20, 2022

issues-383 Create new crate for rusqlite #422

Merged

issues-383 Rewrite examples using sea-query-binder #423

Merged

billy1624 added this to the 0.27.x milestone Aug 24, 2022

tyt2y3 closed this as completed in #416 Sep 3, 2022

ikrivosheev reopened this Sep 3, 2022

ikrivosheev mentioned this issue Sep 3, 2022

issues-383 Remove thread-safe feature #431

Merged

tyt2y3 closed this as completed in #423 Sep 4, 2022

ikrivosheev mentioned this issue Sep 4, 2022

issues-383 Move postgres driver into sea-query-postgres #433

Merged

ikrivosheev mentioned this issue Sep 6, 2022

issues-383 Remove driver #434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate / remove driver in favour of binder #383

Deprecate / remove driver in favour of binder #383

Sytten commented Jul 15, 2022

tyt2y3 commented Jul 17, 2022 •

edited

Loading

ikrivosheev commented Jul 17, 2022

tyt2y3 commented Jul 17, 2022

ikrivosheev commented Jul 17, 2022

tyt2y3 commented Jul 17, 2022 •

edited

Loading

ikrivosheev commented Jul 17, 2022

Sytten commented Jul 17, 2022 •

edited

Loading

ikrivosheev commented Jul 17, 2022

Sytten commented Jul 17, 2022

ikrivosheev commented Jul 17, 2022

tyt2y3 commented Jul 23, 2022

Sytten commented Jul 23, 2022

ikrivosheev commented Jul 25, 2022

ikrivosheev commented Aug 20, 2022 •

edited

Loading

billy1624 commented Aug 23, 2022

billy1624 commented Aug 23, 2022

billy1624 commented Aug 23, 2022 •

edited

Loading

tyt2y3 commented Aug 24, 2022

ikrivosheev commented Aug 25, 2022

ikrivosheev commented Aug 25, 2022

billy1624 commented Aug 26, 2022

Sytten commented Aug 26, 2022

ikrivosheev commented Aug 26, 2022

ikrivosheev commented Sep 3, 2022

ikrivosheev commented Sep 3, 2022

tyt2y3 commented Sep 4, 2022

tyt2y3 commented Sep 4, 2022 •

edited

Loading

ikrivosheev commented Sep 4, 2022

ikrivosheev commented Sep 6, 2022

tyt2y3 commented Sep 6, 2022

Deprecate / remove driver in favour of binder #383

Deprecate / remove driver in favour of binder #383

Comments

Sytten commented Jul 15, 2022

tyt2y3 commented Jul 17, 2022 • edited Loading

ikrivosheev commented Jul 17, 2022

tyt2y3 commented Jul 17, 2022

ikrivosheev commented Jul 17, 2022

tyt2y3 commented Jul 17, 2022 • edited Loading

ikrivosheev commented Jul 17, 2022

Sytten commented Jul 17, 2022 • edited Loading

ikrivosheev commented Jul 17, 2022

Sytten commented Jul 17, 2022

ikrivosheev commented Jul 17, 2022

tyt2y3 commented Jul 23, 2022

Sytten commented Jul 23, 2022

ikrivosheev commented Jul 25, 2022

ikrivosheev commented Aug 20, 2022 • edited Loading

billy1624 commented Aug 23, 2022

billy1624 commented Aug 23, 2022

billy1624 commented Aug 23, 2022 • edited Loading

tyt2y3 commented Aug 24, 2022

ikrivosheev commented Aug 25, 2022

ikrivosheev commented Aug 25, 2022

billy1624 commented Aug 26, 2022

Sytten commented Aug 26, 2022

ikrivosheev commented Aug 26, 2022

ikrivosheev commented Sep 3, 2022

ikrivosheev commented Sep 3, 2022

tyt2y3 commented Sep 4, 2022

tyt2y3 commented Sep 4, 2022 • edited Loading

ikrivosheev commented Sep 4, 2022

ikrivosheev commented Sep 6, 2022

tyt2y3 commented Sep 6, 2022

tyt2y3 commented Jul 17, 2022 •

edited

Loading

tyt2y3 commented Jul 17, 2022 •

edited

Loading

Sytten commented Jul 17, 2022 •

edited

Loading

ikrivosheev commented Aug 20, 2022 •

edited

Loading

billy1624 commented Aug 23, 2022 •

edited

Loading

tyt2y3 commented Sep 4, 2022 •

edited

Loading