#107 Move websocket code into workspace

Merged
dessalines merged 7 commits from move-websocket-to-workspace into main 2 months ago
nutomic commented 2 months ago
Owner

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

Need to clean this up and then actually move websocket into its own workspace
nutomic commented 2 months ago
Poster
Owner

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 2 months ago
Poster
Owner

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 ```
Poster
Owner

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

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 2 months ago
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.

id: ConnectionId,
op: UserOperation,
data: &str,
) -> Pin<Box<dyn Future<Output = Result<String, LemmyError>> + '_>>;
dessalines commented 2 months ago

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

This is really weird to me. Why does a function need to be a type. Why not just call the function.
nutomic commented 2 months ago

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.

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.
pub fn startup(
pool: Pool<ConnectionManager<PgConnection>>,
rate_limiter: RateLimit,
message_handler: MessageHandlerType,
dessalines commented 2 months ago

Why pass in a function here?

Why pass in a function here?
})?;
let user_operation = UserOperation::from_str(&op)?;
let fut = (message_handler)(context, msg.id, user_operation.clone(), data);
dessalines commented 2 months ago

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

Same, this could just call the function instead of referring to the type.
nutomic commented 2 months ago
Poster
Owner

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).

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).
Poster
Owner

Mmmk everything passed, so I'll merge.

Mmmk everything passed, so I'll merge.
dessalines merged commit 442369a041 into main 2 months ago
The pull request has been merged as 442369a041.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.