Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial commit to bump diesel to 2.0.0-rc.0 and see what happens #2257

Closed
wants to merge 5 commits into from

Conversation

chusteven
Copy link
Contributor

@chusteven chusteven commented May 11, 2022

Re: #2234

diesel has released an RC for 2.0 (release notes here). I wanted to see what effort was involved in using this new version of diesel. I'm putting up this PR to collaborate with the maintainers on how we can potentially modularize (or not) this version bump. Some things to watch out for:

Full migration guide here; and bug tracking discussion here

  • All existing connections need to be mutable references
  • diesel-newtypes crate needs to be bumped too
  • Table aliasing support as per original issue

@chusteven
Copy link
Contributor Author

chusteven commented May 11, 2022

One thing I could think to do is cd into each of the workspace crates individually and try building those? Seems like a relatively parallelizable way to do this migration? I saw that the top-level crate has dependencies on some of these workspace ones, but that could be saved for last -- if I'm thinking about this correctly


EDIT: I did the above and it seems like the rest are failing due to incompatibilies with the diesel-derive-newtype crate not yet being compatible with diesel 2.0 (which it looks to be a fair amount of progress on?) As well as what I'm guessing are all the other things that diesel 2.0 breaks (needing mutable references to connection objects, etc.)

The following pass cargo check:

  • api_common
  • apub_lib (after adding "chrono" feature flag to diesel dep)
  • apub_lib_derive
  • db_schema
  • db_views
  • db_views_actor
  • db_views_moderator
  • utils (same as apub_lib)
  • websocket

@Nutomic
Copy link
Member

Nutomic commented May 11, 2022

The correct way to build a single crate in a workspace is cargo check -p lemmy_db_views. Doing it with cd might not work correctly.

The error with Utc::now() can be fixed by enabling the clock feature in chrono. I guess that was previosly enabled implicitly by diesel.

@chusteven
Copy link
Contributor Author

chusteven commented May 11, 2022

The correct way to build a single crate in a workspace is cargo check -p lemmy_db_views. Doing it with cd might not work correctly.

The error with Utc::now() can be fixed by enabling the clock feature in chrono. I guess that was previosly enabled implicitly by diesel.

Thanks! I had to add "chrono" to the features param in the diesel dependency in those Cargo files, but now it works :D I'm inclined to go one by one in each workspace crate to see what can be done here; a quick look at the very many compiler messages suggests that yeah a big blocker will be b/c of diesel-derive-newtype but I'm going to see if I can maybe chip away at some of the other errors and hopefully that helps at least a little

@chusteven chusteven force-pushed the f/bump-diesel-2.0-rc branch 3 times, most recently from 0201546 to bf8f55d Compare May 11, 2022 23:45
@chusteven
Copy link
Contributor Author

Sigh I'm afraid I may have bitten off more than I can chew at the moment; the main thing that makes me say this is that from the root dir, running cargo check still gives all these compiler error messages. Additionally, inside VSCode with the rust-analyzer extension I see some red squigglys that suggest the compiler is still unhappy

These lines in particular

the conn I know should be a mutable reference. But still when I do

stevench@C02YT0D8LVCJ lemmy % cargo build -p lemmy_db_schema
    Finished dev [unoptimized] target(s) in 0.28s

I get a successful build. So I'm definitely out of my depths on both (a) general Rust dev workflows as well as (b) what in the world else could be done here while waiting for diesel-derive-newtype to add support for diesel 2.0.

Anyway happy to keep this PR open, or close it, or -- if some advice can be given -- press onward!

@Nutomic
Copy link
Member

Nutomic commented May 12, 2022

diesel-derive-newtype just got updated yesterday for diesel 2.0.

https://github.com/quodlibetor/diesel-derive-newtype

@dessalines
Copy link
Member

dessalines commented May 12, 2022

Also besides newtype, run cargo +nightly fmt before each commit, because there are formatting errors in the CI: https://cloud.drone.io/LemmyNet/lemmy/3364/1/4

I also use rust-analyzer so you should be fine there, it occasionally needs you to run cargo clean in the root dir, especially if you update deps.

@chusteven
Copy link
Contributor Author

Thanks to both! Okay I'll take another stab at this this weekend -- will try bumping diesel-derive-newtype

Also besides newtype, run cargo +nightly fmt before each commit, because there are formatting errors in the CI

Roger!

I also use rust-analyzer so you should be fine there, it occasionally needs you to run cargo clean in the root dir, especially if you update deps.

Ahhh the occasional cargo clean bringing me back to Java days 😁 with Maven

@Nutomic
Copy link
Member

Nutomic commented May 20, 2022

I was able to apply the upgrade to diesel-derive-newtype 2.0.0-rc.0 on your branch. This fixes most of the errors in db_schema, only a few remain (and tons of warnings). However CI fails, you need to run cargo +nightly fmt --all.

@chusteven
Copy link
Contributor Author

I was able to apply the upgrade to diesel-derive-newtype 2.0.0-rc.0 on your branch. This fixes most of the errors in db_schema, only a few remain (and tons of warnings). However CI fails, you need to run cargo +nightly fmt --all.

Cool; thanks for the tip about the nightly format stuff :) Also sorry I dropped off; but great to hear that a lot of the errors are fixed with the newtype bump! When I find some time I'll be happy to go through and do the rest of the grunt work for the remaining changes from diesel 2.0!

new way to do migrations as suggested by the migration guide; a lot more
compiles now, though I can't figure out this tricky ToSql issue at the
moment
@dessalines
Copy link
Member

This needs a merge from main.

@Nutomic
Copy link
Member

Nutomic commented Aug 24, 2022

Any update on this?

@chusteven
Copy link
Contributor Author

Any update on this?

Ah sorry unfortunately things got crazy and I took a new job 😅 and haven't had much time. I don't know how far this branch has gotten from main so it might not even be worth keeping this PR open :( Otherwise I probably won't find more time to work on this until at least November/December

@dessalines
Copy link
Member

No probs @chusteven . Now that diesel 2.0.0 is released, I should make this a priority, so I'll create my own branch off of this. Your work will not go to waste!

@dessalines
Copy link
Member

Closing in favor of #2452 . Your commits were not wasted, thx for helping out!

@dessalines dessalines closed this Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants