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

feat(workspace_tests): drop path requirement for dev dependencies #3653

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review January 24, 2025 17:56
@dorimedini-starkware dorimedini-starkware self-assigned this Jan 24, 2025
Copy link
Contributor

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions branch from 71692dc to a7c33d0 Compare January 26, 2025 10:47
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch from d22bb0a to d3236c8 Compare January 26, 2025 10:47
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions branch from a7c33d0 to 659e412 Compare January 26, 2025 11:13
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch from d3236c8 to fc62579 Compare January 26, 2025 11:13
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions branch from 659e412 to 2180d1b Compare January 26, 2025 11:58
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch from fc62579 to 95d87bf Compare January 26, 2025 11:58
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions branch from 2180d1b to da7f905 Compare February 12, 2025 10:28
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch from 95d87bf to dc92c94 Compare February 12, 2025 10:28
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions branch from da7f905 to c9f04c5 Compare February 12, 2025 10:51
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch 2 times, most recently from 7cebf66 to cfb0193 Compare February 12, 2025 11:07
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions branch from c9f04c5 to f9a0e6c Compare February 12, 2025 14:06
@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch 2 times, most recently from 6b3e20c to f7c5df3 Compare February 12, 2025 14:39
@dorimedini-starkware dorimedini-starkware changed the base branch from 01-24-feat_workspace_tests_test_that_only_publishable_member_crates_have_versions to main February 12, 2025 14:39
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 19 files at r2.
Reviewable status: 21 of 36 files reviewed, all discussions resolved (waiting on @giladchase and @Itay-Tsabary-Starkware)


workspace_tests/toml_utils.rs line 68 at r3 (raw file):

    pub(crate) fn path_dependencies(&self) -> impl Iterator<Item = String> + '_ {
        self.dependencies.iter().flatten().chain(self.dev_dependencies.iter().flatten()).filter_map(

the path_dependencies function is used by the validate_no_path_dependencies test. now, even dev-dependencies should use workspace=true instead of path="../X".

Code quote:

chain(self.dev_dependencies.iter().flatten())

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 21 of 36 files reviewed, all discussions resolved (waiting on @giladchase and @Itay-Tsabary-Starkware)


workspace_tests/package_integrity_test.rs line 126 at r3 (raw file):

///   dependency, we will not be able to publish starknet_api.
#[test]
fn test_member_dev_dependencies_are_by_path() {

no longer required.
non-publishable crates don't have a version (this is enforced by test_members_have_version_iff_they_are_for_publish),
and we enforce no circular crate dependencies (including self-deps) can exist in test_no_self_dependencies.

So, we don't need path deps anymore (and we don't want them)

Code quote:

/// For smooth publishing: all *local* (workspace member) dependencies in [dev-dependencies] should
/// be via path, and not `workspace = true`.
/// Reason:
/// - `cargo publish` ignores path dependencies in dev-dependencies.
/// - `cargo publish` DOES NOT ignore `workspace = true` dependencies in dev-dependencies, even if
///   the workspace toml defines the dependency via path.
/// - We do not need dev-dependencies published when publishing a crate.
/// - We sometimes use self-references in dev-dependencies to activate features of self in tests.
///   For example, starknet_api needs it's "testing" feature activated in test mode, so it has a
///   dependency on self (starknet_api) in dev-dependencies. If we fail to ignore this self
///   dependency, we will not be able to publish starknet_api.
#[test]
fn test_member_dev_dependencies_are_by_path() {

@dorimedini-starkware dorimedini-starkware force-pushed the 01-24-feat_workspace_tests_drop_path_requirement_for_dev_dependencies branch from f7c5df3 to 74cbcfc Compare February 13, 2025 12:58
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.

3 participants