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

Specify dependencies and metadata for entire workspace #2565

Merged
merged 6 commits into from
Nov 17, 2022

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Nov 16, 2022

We are now using Rust 1.64 in CI, so we can use this feature.

@Nutomic Nutomic force-pushed the workspace-dependencies branch 4 times, most recently from 83f3b78 to 50b72c6 Compare November 16, 2022 14:27
Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using any tool to analyze these, cargo deps maybe?

captcha = "0.0.9"
anyhow = "1.0.66"
diesel_ltree = "0.3.0"
typed-builder = "0.10.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't a lot of these dependencies use workspace = true also? Almost all of them are included in utils.

edit: okay I spose this seems good, since you're preferring to specify them higher up, then use them lower.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I excluded deps which are only used in one of our crates. Seems like it would be more annoying than helpful to specify them in two places in that case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And only tools im using are cargo check/cargo tree.

webpage = { version = "1.4.0", default-features = false, features = ["serde"], optional = true }
regex = "1.6.0"
encoding = { version = "0.2.33", optional = true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more organized to make everything in all of these workspace crates use workspace = true, and move all the deps to the top level. Not sure.

use crate::newtypes::{CommentId, DbUrl, LanguageId, LtreeDef, PersonId, PostId};
#[cfg(feature = "full")]
use crate::newtypes::LtreeDef;
use crate::newtypes::{CommentId, DbUrl, LanguageId, PersonId, PostId};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this is gonna fail CI check.

@dessalines
Copy link
Member

dessalines commented Nov 16, 2022

Found something interesting, cargo deny, testing it out now:

cargo install --locked cargo-deny && cargo deny init && cargo deny check

- name: lemmy_api_common doesnt depend on diesel
image: rust:1.64-buster
commands:
- "! cargo tree -p lemmy_api_common --no-default-features -i diesel"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exclamation point isn't necessary right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its necessary because we want to ensure that the command returns an error via exit code. This means the diesel dependency was not found, which is the goal in this case. Crates which pull in lemmy_api_common to access api structs (eg lemmybb) shouldnt have to compile diesel.

@Nutomic
Copy link
Member Author

Nutomic commented Nov 17, 2022

Some of the database tests are failing randomly, happened in other PRs as well:

failures:


---- impls::actor_language::tests::test_community_languages stdout ----

thread 'main' panicked at 'assertion failed: `(left == right)`

  left: `[LanguageId(37), LanguageId(47), LanguageId(135)]`,

 right: `[LanguageId(135), LanguageId(47), LanguageId(37)]`', crates/db_schema/src/impls/actor_language.rs:480:5

stack backtrace:

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


---- impls::actor_language::tests::test_default_post_language stdout ----

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(UniqueViolation, "duplicate key value violates unique constraint \"site_name_key\"")', crates/db_schema/src/impls/actor_language.rs:352:53

stack backtrace:

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


---- impls::actor_language::tests::test_site_languages stdout ----

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(UniqueViolation, "duplicate key value violates unique constraint \"site_name_key\"")', crates/db_schema/src/impls/actor_language.rs:352:53

stack backtrace:

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


---- impls::actor_language::tests::test_user_languages stdout ----

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(UniqueViolation, "duplicate key value violates unique constraint \"site_name_key\"")', crates/db_schema/src/impls/actor_language.rs:352:53

stack backtrace:

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.



failures:

    impls::actor_language::tests::test_community_languages

    impls::actor_language::tests::test_default_post_language

    impls::actor_language::tests::test_site_languages

    impls::actor_language::tests::test_user_languages


test result: FAILED. 23 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out; finished in 15.24s

@Nutomic Nutomic merged commit df7809f into main Nov 17, 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.

2 participants