LemmyNet
/
lemmy
3
2
Fork 3
Code Pull Requests 1 Releases Wiki Activity

Move code into different workspaces #94

Closed
nutomic wants to merge 2 commits from create-workspaces into main
pull from: create-workspaces
merge into: LemmyNet:main
Conversation 7 Commits 2 Files Changed 14 +1129 -977
nutomic commented 2020-09-01 17:03:40 +00:00
Owner
Copy Link

Moving api structs and rate limit code into separate workspaces was relatively easy, and that part could already be merged. But its not working with websocket, mainly due to LemmyContext. And api/apub are even more complicated.

Moving api structs and rate limit code into separate workspaces was relatively easy, and that part could already be merged. But its not working with websocket, mainly due to LemmyContext. And api/apub are even more complicated.
dessalines reviewed 2020-09-01 17:08:55 +00:00
server/src/api/user.rs Outdated
@ -8,3 +2,1 @@
APIError,
Perform,
},
api::{check_slurs, claims::Claims, get_user_from_jwt, get_user_from_jwt_opt, is_admin, Perform},
dessalines commented 2020-09-01 17:08:55 +00:00
Owner
Copy Link

Run cargo +nightly fmt on this again.

Run `cargo +nightly fmt` on this again.
dessalines commented 2020-09-01 17:13:32 +00:00
Owner
Copy Link

I think this also needs changes to the Dockerfiles, but https://github.com/LemmyNet/lemmy/pull/1112 should be merged first.

I think this also needs changes to the `Dockerfiles`, but https://github.com/LemmyNet/lemmy/pull/1112 should be merged first.
nutomic commented 2020-09-01 17:38:16 +00:00
Author
Owner
Copy Link

I can make a PR with only the api_structs and rate_limit workspaces (plus cargo fmt and docker stuff). But the question is, how do we proceed with moving the rest into workspaces? Just run these commands to see the problem with circular dependencies:

  • rg websocket:: src/api/
  • rg apub:: src/api/
  • rg api:: src/apub/
  • rg websocket:: src/apub/

If we want to put api and apub into separate workspaces, then they cant depend on each other in a circle. Ideally they wouldnt depend on each other at all so they can be compiled in parallel.

I can make a PR with only the api_structs and rate_limit workspaces (plus cargo fmt and docker stuff). But the question is, how do we proceed with moving the rest into workspaces? Just run these commands to see the problem with circular dependencies: - rg websocket:: src/api/ - rg apub:: src/api/ - rg api:: src/apub/ - rg websocket:: src/apub/ If we want to put api and apub into separate workspaces, then they cant depend on each other in a circle. Ideally they wouldnt depend on each other at all so they can be compiled in parallel.
dessalines commented 2020-09-01 22:06:28 +00:00
Owner
Copy Link

Just to comment on a few of these:

rg websocket:: src/api/
rg apub:: src/api/

The API needs to send websocket and apub messages, so I don't see a way around it being dependent on those, its gotta be built after. The API is kind of our central hub that requires everything.

rg api:: src/apub/

❯ rg api:: src/apub/
src/apub/user.rs
2:  api::{check_slurs, check_slurs_opt},

src/apub/inbox/activities/create.rs
2:  api::comment::send_local_notifs,

src/apub/inbox/activities/update.rs
2:  api::comment::send_local_notifs,

src/apub/community.rs
2:  api::{check_slurs, check_slurs_opt},

src/apub/post.rs
2:  api::check_slurs,

Check_slurs and check_slurs_opt are just helper functions for slur_check that wrap it in an APIError, they aren't really needed in apub.

send_local_notifs could be moved to lemmy_db, I think the only other dependency it has is send_email which is in lemmy_utils (which I think lemmy_db already imports). So we could probably remove this circular dependency of api <-> apub, and have it just be api -> apub

rg websocket:: src/apub/

This one's unavoidable too, the apub library does depend on websocket since it has to send websocket messages.

So I think the best we could do here is this, which means both are reliant on websocket.

websocket <-api
  ^           |
  |           |
 apub <-----/
Just to comment on a few of these: > rg websocket:: src/api/ > rg apub:: src/api/ The API needs to send websocket and apub messages, so I don't see a way around it being dependent on those, its gotta be built after. The API is kind of our central hub that requires everything. > rg api:: src/apub/ ``` ❯ rg api:: src/apub/ src/apub/user.rs 2: api::{check_slurs, check_slurs_opt}, src/apub/inbox/activities/create.rs 2: api::comment::send_local_notifs, src/apub/inbox/activities/update.rs 2: api::comment::send_local_notifs, src/apub/community.rs 2: api::{check_slurs, check_slurs_opt}, src/apub/post.rs 2: api::check_slurs, ``` Check_slurs and check_slurs_opt are just helper functions for `slur_check` that wrap it in an APIError, they aren't really needed in apub. send_local_notifs could be moved to `lemmy_db`, I think the only other dependency it has is `send_email` which is in `lemmy_utils` (which I think `lemmy_db` already imports). So we could probably remove this circular dependency of `api <-> apub`, and have it just be `api -> apub` > rg websocket:: src/apub/ This one's unavoidable too, the apub library does depend on websocket since it has to send websocket messages. So I think the best we could do here is this, which means both are reliant on websocket. ``` websocket <-api ^ | | | apub <-----/ ```
nutomic commented 2020-09-02 11:49:57 +00:00
Author
Owner
Copy Link

I fixed some of the problems by moving LemmyContext and Perform into lemmy_websocket, even though that doesnt make much sense with the name. But now the problem is this:

--> lemmy_websocket/src/chat_server.rs:453:65
UserOperation::CreateCommentLike => do_user_operation::<CreateCommentLike>(args).await,

the trait `Perform` is not implemented for `lemmy_api_structs::comment::CreateCommentLike`

So again the problem is that websocket depends on API. I'm thinking that we need to add some kind of generic message handler, which would allow the different modules to interact without depending on each other. But I'm not sure how to do that in Rust.

Edit: I think this from the Rust book might work, but it would make things more complicated and possibly affect performance.

Also, I'm wondering whether we could possibly split up websocket into one part for receiving, and an independent part for sending messages. Maybe that could resolve the dependency issue.

I fixed some of the problems by moving LemmyContext and Perform into lemmy_websocket, even though that doesnt make much sense with the name. But now the problem is this: ``` --> lemmy_websocket/src/chat_server.rs:453:65 UserOperation::CreateCommentLike => do_user_operation::<CreateCommentLike>(args).await, the trait `Perform` is not implemented for `lemmy_api_structs::comment::CreateCommentLike` ``` So again the problem is that websocket depends on API. I'm thinking that we need to add some kind of generic message handler, which would allow the different modules to interact without depending on each other. But I'm not sure how to do that in Rust. Edit: I think [this from the Rust book](https://doc.rust-lang.org/book/ch16-02-message-passing.html) might work, but it would make things more complicated and possibly affect performance. Also, I'm wondering whether we could possibly split up websocket into one part for receiving, and an independent part for sending messages. Maybe that could resolve the dependency issue.
nutomic commented 2020-09-02 12:23:16 +00:00
Author
Owner
Copy Link

Anyway if you think splitting out the api structs and rate limiter makes sense, we can just merge that part for now. Then we can focus on reducing the interdependencies between api, apub and websocket (while keeping them all inside lemmy_server for now).

Anyway if you think splitting out the api structs and rate limiter makes sense, we can just merge that part for now. Then we can focus on reducing the interdependencies between api, apub and websocket (while keeping them all inside lemmy_server for now).
dessalines commented 2020-09-02 13:46:48 +00:00
Owner
Copy Link

Yeah that's fine for now. I'm doing some docker caching testing this morning, but after that I'll merge this as is.

Yeah that's fine for now. I'm doing some docker caching testing this morning, but after that I'll merge this as is.
dessalines commented 2020-09-02 23:20:58 +00:00
Owner
Copy Link

Okay I've separated that commit out here, and fixed the dockerfiles https://github.com/LemmyNet/lemmy/pull/1113

Okay I've separated that commit out here, and fixed the dockerfiles https://github.com/LemmyNet/lemmy/pull/1113
nutomic closed this pull request 2020-09-03 13:59:38 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#94
No description provided.
Delete Branch "create-workspaces"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?