Move websocket code into workspace #107

nutomic · 2020-09-22T18:59:46Z

nutomic commented

2020-09-22 18:59:46 +00:00

Need to clean this up and then actually move websocket into its own workspace

nutomic commented

2020-09-23 15:03:32 +00:00

Got it, websocket, api and apub are all in separate workspaces now! It might not benefit compile times much, because the workspaces depend on each other linearly and dont allow for parallel compilation.

I used cargo-udeps to get rid of unused dependencies, but it doesnt seem to work on the main project. So we might have to try manually which deps can be removed.

For lemmy_api we should split up lib.rs, and probably move the stuff there into an internals folder.

Got it, websocket, api and apub are all in separate workspaces now! It might not benefit compile times much, because the workspaces depend on each other linearly and dont allow for parallel compilation. I used cargo-udeps to get rid of unused dependencies, but it doesnt seem to work on the main project. So we might have to try manually which deps can be removed. For lemmy_api we should split up lib.rs, and probably move the stuff there into an `internals` folder.

nutomic commented

2020-09-23 15:19:06 +00:00

Here are some measurements for compilation time, done with cargo clean && RUSTC_WRAPPER= cargo +nightly build -Ztimings. There is a lot of variance, but it looks like compilation time has not improved from this (as expected).

on main: 3m 45s, 3m 58s, 4m 02s
after moving websocket to workspace: 3m 29s, 4m 22s, 3m 55s
after moving api, apub to workspaces: 4m 09s, 3m 35s, 3m 43s

Here are some measurements for compilation time, done with `cargo clean && RUSTC_WRAPPER= cargo +nightly build -Ztimings`. There is a lot of variance, but it looks like compilation time has not improved from this (as expected). ``` on main: 3m 45s, 3m 58s, 4m 02s after moving websocket to workspace: 3m 29s, 4m 22s, 3m 55s after moving api, apub to workspaces: 4m 09s, 3m 35s, 3m 43s ```

dessalines commented

2020-09-23 20:37:40 +00:00

Going through this now, but just a note that it failed on travis here: https://travis-ci.org/github/LemmyNet/lemmy/builds/729704932#L351

dessalines reviewed 2020-09-23 20:49:47 +00:00

dessalines left a comment

Its kinda tough to go through this one because most of it is just a re-org, but unfortunately its blocks of code, not files, so git isn't smart enough to do simple moved files.

But once the tests pass on travis, I'll test on the federation test instances too, since travis doesn't do websocket testing.

Its kinda tough to go through this one because most of it is just a re-org, but unfortunately its blocks of code, not files, so git isn't smart enough to do simple moved files. But once the tests pass on travis, I'll test on the federation test instances too, since travis doesn't do websocket testing.

lemmy_websocket/src/chat_server.rs

						
				@ -0,0 +33,4 @@

				  id: ConnectionId,

				  op: UserOperation,

				  data: &str,

				) -> Pin<Box<dyn Future<Output = Result<String, LemmyError>> + '_>>;

dessalines commented

2020-09-23 20:43:45 +00:00

This is really weird to me. Why does a function need to be a type. Why not just call the function.

nutomic commented

2020-09-24 10:56:36 +00:00

This was the only option I found to remove the dependency of websocket on api. If I left it as a normal function call, then websocket would depend on api, while api also depends on websocket. So it would be impossible to move them into separate crates.

lemmy_websocket/src/chat_server.rs

						
				@ -0,0 +82,4 @@

				  pub fn startup(

				    pool: Pool<ConnectionManager<PgConnection>>,

				    rate_limiter: RateLimit,

				    message_handler: MessageHandlerType,

dessalines commented

2020-09-23 20:44:45 +00:00

Why pass in a function here?

lemmy_websocket/src/chat_server.rs

						
				@ -0,0 +371,4 @@

				      })?;

				      let user_operation = UserOperation::from_str(&op)?;

				      let fut = (message_handler)(context, msg.id, user_operation.clone(), data);

dessalines commented

2020-09-23 20:45:52 +00:00

Same, this could just call the function instead of referring to the type.

nutomic commented

2020-09-24 10:55:26 +00:00

Just pushed a commit to fix the Docker builds (and Travis). I also removed unused dependencies in the main project (where the code that used it was moved into workspaces).

dessalines commented

2020-09-24 13:53:06 +00:00

Mmmk everything passed, so I'll merge.

dessalines referenced this issue from a commit

2020-09-24 13:53:22 +00:00

Move websocket code into workspace (#107) Adjust dockerfiles, fix cargo.toml and remove unused deps Merge branch 'main' into move-websocket-to-workspace Move api code into workspace Move apub to separate workspace Move websocket code into separate workspace Some code cleanup Remove websocket dependency on API Co-authored-by: Felix Ableitner <me@nutomic.com> Reviewed-on: https://yerbamate.dev/LemmyNet/lemmy/pulls/107

dessalines merged commit 442369a041 into main

2020-09-24 13:53:22 +00:00

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: LemmyNet/lemmy#107