Split code into cargo workspaces #67

nutomic · 2020-07-07T18:33:08Z

nutomic commented

2020-07-07 18:33:08 +00:00

It looks like clean build time increased a lot since we merged the federation branch. In my test it took 2:20 minutes. This PR decreases it to 1:28.

Still need to fix the tests.

Some more things to try:

remove lemmy_db dependency on lemmy_utils (so they can be compiled in parallel)
remove lemmy_utils dependency on actix_web
move api and apub into separate workspaces
remove diesel dependency on chrono

It looks like clean build time increased a lot since we merged the federation branch. In my test it took 2:20 minutes. This PR decreases it to 1:28. Still need to fix the tests. Some more things to try: - remove lemmy_db dependency on lemmy_utils (so they can be compiled in parallel) - remove lemmy_utils dependency on actix_web - move api and apub into separate workspaces - remove diesel dependency on chrono

nutomic commented

2020-07-07 20:16:16 +00:00

Fixed the test imports, but tests are still failing because the relative path config/defaults.hjson is not correct. I guess the best fix would be to make it so that tests dont need those files at all.

Note that tests need to be run with cargo test --all now, and format with cargo fmt --all.

Edit:: Not sure if this PR in its current state actually speeds things up, seeing 108s on master and 103s on this branch.

Fixed the test imports, but tests are still failing because the relative path `config/defaults.hjson` is not correct. I guess the best fix would be to make it so that tests dont need those files at all. Note that tests need to be run with `cargo test --all` now, and format with `cargo fmt --all`. Edit:: Not sure if this PR in its current state actually speeds things up, seeing 108s on master and 103s on this branch.

nutomic changed title from ~~WIP: split code into cargo workspaces~~ to split code into cargo workspaces

2020-07-09 13:41:35 +00:00

nutomic changed title from ~~split code into cargo workspaces~~ to Split code into cargo workspaces

2020-07-09 13:41:40 +00:00

nutomic commented

2020-07-09 13:49:29 +00:00

@dessalines Fixed the tests, rebased and ready to review. The first commit just moves files around and changes imports, so its much more important to review the second commit. In particular:

activity table uses text instead of jsonb now to remove that dependency, seems a bit cleaner (but we could revert that as it doesnt affect compile time at all)
test for lemmy_db::activity is failing because of the above change
claims and jwt handling moved from db to api
migrations moved out of the db
federation tests are completely broken for some reason

To be clear, even though this doesnt improve compilation time, I still think its very useful to merge this, as it makes for better separation of the code into mostly independent units.

@dessalines Fixed the tests, rebased and ready to review. The first commit just moves files around and changes imports, so its much more important to review the second commit. In particular: - activity table uses text instead of jsonb now to remove that dependency, seems a bit cleaner (but we could revert that as it doesnt affect compile time at all) - test for lemmy_db::activity is failing because of the above change - claims and jwt handling moved from db to api - migrations moved out of the db - federation tests are completely broken for some reason To be clear, even though this doesnt improve compilation time, I still think its very useful to merge this, as it makes for better separation of the code into mostly independent units.

dessalines commented

2020-07-09 15:25:52 +00:00

I'll test / review this later today, but ya I don't wanna merge anything that breaks the tests, that's gotta be figured out.

nutomic commented

2020-07-09 16:00:36 +00:00

I think all thats needed is writing a migration for activity.data, converting it to TEXT. Based on this error: ERROR: column "data" is of type jsonb but expression is of type text at character 76.

I tried the following byt its not working:

ALTER TABLE activity ALTER COLUMN data TYPE TEXT USING (data::TEXT);
# Failed with: operator does not exist: text ->> text

I think all thats needed is writing a migration for `activity.data`, converting it to TEXT. Based on this error: `ERROR: column "data" is of type jsonb but expression is of type text at character 76`. I tried the following byt its not working: ``` ALTER TABLE activity ALTER COLUMN data TYPE TEXT USING (data::TEXT); # Failed with: operator does not exist: text ->> text ```

dessalines reviewed 2020-07-09 19:19:40 +00:00

dessalines left a comment

Looks good, I'm guessing the tests are failing due to the activity jsonb column stuff.

Also I'm not sure why but the docker-compose.ymls are all broken.

Looks good, I'm guessing the tests are failing due to the activity jsonb column stuff. Also I'm not sure why but the `docker-compose.yml`s are all broken.

docs/src/contributing_tests.md Outdated

						
				@ -8,3 +8,3 @@

				```bash

				psql -U lemmy -c "DROP SCHEMA public CASCADE; CREATE SCHEMA public;"

				export DATABASE_URL=postgres://lemmy:password@localhost:5432/lemmy

				export LEMMY_DATABASE_URL=postgres://lemmy:password@localhost:5432/lemmy

dessalines commented

2020-07-09 18:58:44 +00:00

This does need to be DATABASE_URL, because that's what diesel needs to run the migrations.

This does need to be `DATABASE_URL`, because that's what diesel needs to run the migrations.

server/lemmy_db/Cargo.toml Outdated

						
				@ -0,0 +1,15 @@

				[package]

				name = "lemmy_db"

				version = "0.1.0"

				authors = ["Felix Ableitner <me@nutomic.com>"]

dessalines commented

2020-07-09 18:59:35 +00:00

add me too plz

nutomic commented

2020-07-10 00:11:16 +00:00

Of course, this is just what it auto-generated. Tbh I'm not sure its a good idea to have my email listed there in plaintext. Maybe we should put dev@lemmy.ml or something like that?

Of course, this is just what it auto-generated. Tbh I'm not sure its a good idea to have my email listed there in plaintext. Maybe we should put `dev@lemmy.ml` or something like that?

dessalines commented

2020-07-10 00:37:24 +00:00

Ya, sounds good.

server/lemmy_db/src/comment.rs Outdated

						
				@ -0,0 +76,4 @@

				  pub fn update_ap_id(

				    conn: &PgConnection,

				    comment_id: i32,

				    apub_id: String,

dessalines commented

2020-07-09 19:03:28 +00:00

Ah I see, this got moved to the other workspace.

nutomic commented

2020-07-10 00:12:26 +00:00

Yep it was one of the few apub things in the database, so it seemed cleaner to remove the apub dependency from the db.

server/lemmy_db/src/lib.rs Outdated

						
				@ -0,0 +124,4 @@

				}

				pub fn get_database_url_from_env() -> Result<String, VarError> {

				  env::var("LEMMY_DATABASE_URL")

dessalines commented

2020-07-09 19:07:59 +00:00

Is there any way to import the settings here? This is less than ideal bc if we ever reformat the settings hjson, we'll have to track this down at run-time.

Is there any way to import the settings here? This is less than ideal bc if we ever reformat the settings `hjson`, we'll have to track this down at run-time.

nutomic commented

2020-07-10 00:16:13 +00:00

Then I would probably have to make a separate workspace just for settings. This way seems much cleaner because we want things to be in parallel as much as possible.

But I will add a comment about that or something.

Then I would probably have to make a separate workspace just for settings. This way seems much cleaner because we want things to be in parallel as much as possible. But I will add a comment about that or something.

dessalines commented

2020-07-10 00:38:14 +00:00

Ya, a comment is good enough for now. Surprising that the settings doesn't get used in both of these.

nutomic commented

2020-07-10 14:03:38 +00:00

Actually I thought I removed the check of LEMMY_DATABASE_URL in settings, thats not needed. Will get rid of that, so DATABASE_URL here in db will be the only env var for that.

Actually I thought I removed the check of `LEMMY_DATABASE_URL` in settings, thats not needed. Will get rid of that, so `DATABASE_URL` here in db will be the only env var for that.

server/lemmy_db/src/schema.rs Outdated

						
				@ -0,0 +2,4 @@

				    activity (id) {

				        id -> Int4,

				        user_id -> Int4,

				        data -> Text,

dessalines commented

2020-07-09 19:09:35 +00:00

I don't think you can just overwrite it like that, and either way it gets re-written every time you run a migration.

server/lemmy_utils/Cargo.toml Outdated

						
				@ -0,0 +1,23 @@

				[package]

				name = "lemmy_utils"

				version = "0.1.0"

				authors = ["Felix Ableitner <me@nutomic.com>"]

dessalines commented

2020-07-09 19:10:54 +00:00

same

server/src/api/claims.rs Outdated

						
				@ -0,0 +70,4 @@

				    let claims: Claims = Claims::decode(&jwt).expect("Invalid token").claims;

				    User_::read(&conn, claims.id)

				  }

				}

dessalines commented

2020-07-09 19:12:40 +00:00

This does make sense to be split out. Soon I'm gonna remove almost all this stuff from the jwt, and just pull it, in order to make the settings more portable.

nutomic commented

2020-07-10 14:55:04 +00:00

Updated. See the last commit for details.

However, test_mentions_regex is failing, because WEBFINGER_COMMUNITY_REGEX reads a field from settings. Problem is that during tests, the path config/defaults.hjson is interpreted as relative to the lemmy_utils folder, meaning it tries to read from server/lemmy_utils/config/defaults.hjson (which doesnt exist). So either we need to make it so the test doesnt read from settings, or we add another config file in that location.

Federation tests are fixed, except "create test comment" is failing for some reason (I will investigate later).

I also had to change all occurences of LEMMY_DATABASE_URL to DATABASE_URL, which is a breaking change. I could also add a check for the old env var to stay backwards compatible.

Updated. See the last commit for details. However, `test_mentions_regex` is failing, because `WEBFINGER_COMMUNITY_REGEX` reads a field from settings. Problem is that during tests, the path `config/defaults.hjson` is interpreted as relative to the lemmy_utils folder, meaning it tries to read from `server/lemmy_utils/config/defaults.hjson` (which doesnt exist). So either we need to make it so the test doesnt read from settings, or we add another config file in that location. Federation tests are fixed, except "create test comment" is failing for some reason (I will investigate later). I also had to change all occurences of `LEMMY_DATABASE_URL` to `DATABASE_URL`, which is a breaking change. I could also add a check for the old env var to stay backwards compatible.

dessalines commented

2020-07-10 15:45:59 +00:00

However, test_mentions_regex is failing, because WEBFINGER_COMMUNITY_REGEX reads a field from settings. Problem is that during tests, the path config/defaults.hjson is interpreted as relative to the lemmy_utils folder, meaning it tries to read from server/lemmy_utils/config/defaults.hjson (which doesnt exist).

Can't we just move these two below up a dir? Or make sure its possible to run the utils tests from the parent dir. Or possibly symlinks might work.

static CONFIG_FILE_DEFAULTS: &str = "config/defaults.hjson";
static CONFIG_FILE: &str = "config/config.hjson";

I also had to change all occurences of LEMMY_DATABASE_URL to DATABASE_URL, which is a breaking change. I could also add a check for the old env var to stay backwards compatible.

That shouldn't be necessary, its fine as it was. DATABASE_URL is something that only diesel needs.

Also all the docker builds fail w/ this:

-----
 > [rust 9/13] RUN cargo build:
#18 1.109 error: failed to read `/app/server/lemmy_utils/Cargo.toml`
#18 1.109
#18 1.109 Caused by:
#18 1.109   No such file or directory (os error 2)
------

> However, test_mentions_regex is failing, because WEBFINGER_COMMUNITY_REGEX reads a field from settings. Problem is that during tests, the path config/defaults.hjson is interpreted as relative to the lemmy_utils folder, meaning it tries to read from server/lemmy_utils/config/defaults.hjson (which doesnt exist). Can't we just move these two below up a dir? Or make sure its possible to run the utils tests from the parent dir. Or possibly symlinks might work. ``` static CONFIG_FILE_DEFAULTS: &str = "config/defaults.hjson"; static CONFIG_FILE: &str = "config/config.hjson"; ``` > I also had to change all occurences of LEMMY_DATABASE_URL to DATABASE_URL, which is a breaking change. I could also add a check for the old env var to stay backwards compatible. That shouldn't be necessary, its fine as it was. DATABASE_URL is something that only diesel needs. Also all the docker builds fail w/ this: ``` ----- > [rust 9/13] RUN cargo build: #18 1.109 error: failed to read `/app/server/lemmy_utils/Cargo.toml` #18 1.109 #18 1.109 Caused by: #18 1.109 No such file or directory (os error 2) ------ ```

nutomic commented

2020-07-10 17:22:25 +00:00

Should be good now, all tests are passing (including federation). Please consider squashing before merging.

dessalines referenced this issue from a commit

2020-07-10 18:15:42 +00:00

Split code into cargo workspaces (#67) More fixes - fixed docker builds - fixed mentions regex test - fixed DATABASE_URL stuff - change schema path in diesel.toml Address review comments - add jsonb column back into activity table - remove authors field from cargo.toml - adjust LEMMY_DATABASE_URL env var usage - rename all occurences of LEMMY_DATABASE_URL to DATABASE_URL Decouple utils and db Split code into cargo workspaces Co-authored-by: Felix Ableitner <me@nutomic.com> Reviewed-on: https://yerbamate.dev/LemmyNet/lemmy/pulls/67

dessalines merged commit 80aef61aed into master

2020-07-10 18:15:42 +00:00

nutomic deleted branch workspaces

2020-07-10 19:41:50 +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#67