chore(ci): workspace version alignment and tests

[dorimedini-starkware]

This change is 

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.631 ms 66.103 ms 66.785 ms]
change: [-7.7188% -4.3248% -1.2920%] (p = 0.01 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.447 ms 65.528 ms 65.625 ms]
change: [-9.1964% -5.7698% -2.6816%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severe

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.39%. Comparing base (091490f) to head (b721a56).
Report is 6 commits behind head on main-v0.13.2.

Files	Patch %	Lines
workspace_tests/version_integrity_test.rs	80.95%	4 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.2     #536      +/-   ##
================================================
- Coverage         76.45%   76.39%   -0.07%     
================================================
  Files               314      316       +2     
  Lines             34205    34285      +80     
  Branches          34205    34285      +80     
================================================
+ Hits              26152    26191      +39     
- Misses             5775     5817      +42     
+ Partials           2278     2277       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.895 ms 66.000 ms 66.143 ms]
change: [-7.7058% -4.3688% -1.4415%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

[nadin-Starkware]

Reviewed 1 of 38 files at r1, 2 of 3 files at r2.
Reviewable status: 3 of 38 files reviewed, all discussions resolved (waiting on @dafnamatsry, @dan-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @noaov1, and @Yoni-Starkware)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [66.274 ms 66.342 ms 66.420 ms]
change: [-8.6048% -5.5112% -2.8055%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe

[dorimedini-starkware]

Reviewed 3 of 38 files at r1, all commit messages.
Reviewable status: 5 of 38 files reviewed, all discussions resolved (waiting on @dafnamatsry, @dan-starkware, @elintul, @giladchase, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

[giladchase]

Reviewable status: 5 of 38 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry, @dan-starkware, @dorimedini-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

.github/workflows/main.yml line 97 at r3 (raw file):

    steps:
      - uses: actions/checkout@v4
      - uses: ./.github/actions/install_rust

Unrelated nonblocking: what does this trigger? we used to use dtolnay/rust-toolchain, why did switch?

Code quote:

     - uses: ./.github/actions/install_rust

---

workspace_tests/version_integrity_test.rs line 3 at r3 (raw file):

use std::collections::HashMap;

use once_cell::sync::Lazy;

U can use the recently added std::sync::OnceLock and save the extra dep, we already started using it for new lazy static in a bunch of crates.

Code quote:

use once_cell::sync::Lazy;

---

workspace_tests/version_integrity_test.rs line 37 at r3 (raw file):

static ROOT_TOML: Lazy<CargoToml> = Lazy::new(|| {
    let root_toml: CargoToml = toml::from_str(include_str!("../Cargo.toml")).unwrap();

Do we want to use env::var("CARGO_MANIFEST_DIR") here instead of ..? That is, use absolute path instead of relative

Code quote:

    let root_toml: CargoToml = toml::from_str(include_str!("../Cargo.toml")).unwrap();

---

workspace_tests/version_integrity_test.rs line 42 at r3 (raw file):

impl<'a> CargoToml {
    fn local_crates(&'a self) -> impl Iterator<Item = LocalCrate> + 'a {

Suggestion:

impl CargoToml {
    fn local_crates(&self) -> impl Iterator<Item = LocalCrate> + '_ {

[dorimedini-starkware]

Reviewable status: 5 of 38 files reviewed, all discussions resolved (waiting on @dafnamatsry, @dan-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

.github/workflows/main.yml line 97 at r3 (raw file):

Previously, giladchase wrote…

Unrelated nonblocking: what does this trigger? we used to use dtolnay/rust-toolchain, why did switch?

so we can globally control the rust toolchain we use in the CI; going forward we will probably want to stop using stable on release branches, and fix a version (so the occasional bugfix PR won't have to include clippy fixes due to stable rust upgrade)

---

workspace_tests/version_integrity_test.rs line 3 at r3 (raw file):

Previously, giladchase wrote…

U can use the recently added std::sync::OnceLock and save the extra dep, we already started using it for new lazy static in a bunch of crates.

with OnceLock don't I have to set the value in each test?

---

workspace_tests/version_integrity_test.rs line 37 at r3 (raw file):

Previously, giladchase wrote…

Do we want to use env::var("CARGO_MANIFEST_DIR") here instead of ..? That is, use absolute path instead of relative

Done, though we still need ../Cargo.toml (CARGO_MANIFEST_DIR is ~/workspace/sequencer/workspace_tests)

---

workspace_tests/version_integrity_test.rs line 42 at r3 (raw file):

impl<'a> CargoToml {
    fn local_crates(&'a self) -> impl Iterator<Item = LocalCrate> + 'a {

thanks!!
can you explain why this is needed at all?

[giladchase]

Reviewable status: 5 of 38 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, @dorimedini-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 42 at r3 (raw file):

impl<'a> CargoToml {
    fn local_crates(&'a self) -> impl Iterator<Item = LocalCrate> + 'a {

Rationale: I found myself a bit confused by local_crates() at first glance.
This makes it clearer that we're picking out dependencies with a path, which I missed when going over it.

Maybe workspace_crates() also works, but that could be assuming too much about those deps at this point, considering that you're testing that specifically in this module.

Suggestion:

    fn path_dependencies(&'a self) -> impl Iterator<Item = LocalCrate> + 'a {

---

workspace_tests/version_integrity_test.rs line 71 at r3 (raw file):

        );
    }
}

Making sure i understand: this goes over all dependencies that have a path attributes and ensures they are in fact crates included in the workspace?
So in essence, we're checking that none of our crates what removed from the workspace by mistake?
(Or maybe also to guard against ppl pushing temporary locally patched external deps?)

If that is the case, maybe we should change _local_dependencies_ to something like _local_path_dependencies_ to make it clear, initially I thought it was a mistake because I thought it was checking that all deps are members, which is of course not the intention.

Code quote:

fn test_local_dependencies_are_members() {
    for LocalCrate { path, name, .. } in ROOT_TOML.local_crates() {
        assert!(
            ROOT_TOML.workspace.members.contains(&path),
            "Crate '{name}' at path '{path}' is not a member of the workspace."
        );
    }
}

[giladchase]

Reviewable status: 5 of 38 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, @dorimedini-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 37 at r3 (raw file):

Previously, dorimedini-starkware wrote…

Done, though we still need ../Cargo.toml (CARGO_MANIFEST_DIR is ~/workspace/sequencer/workspace_tests)

Rightt, shame there isn't a CARGO_WORKSPACE_DIR

---

workspace_tests/version_integrity_test.rs line 42 at r3 (raw file):

Previously, dorimedini-starkware wrote…

thanks!!
can you explain why this is needed at all?

No logical reason, just complicated lifetime ellision that isn't stable yet.

[dorimedini-starkware]

Reviewable status: 5 of 38 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, @elintul, @giladchase, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 71 at r3 (raw file):

Previously, giladchase wrote…

Making sure i understand: this goes over all dependencies that have a path attributes and ensures they are in fact crates included in the workspace?
So in essence, we're checking that none of our crates what removed from the workspace by mistake?
(Or maybe also to guard against ppl pushing temporary locally patched external deps?)

If that is the case, maybe we should change _local_dependencies_ to something like _local_path_dependencies_ to make it clear, initially I thought it was a mistake because I thought it was checking that all deps are members, which is of course not the intention.

this test checks that all path-deps are members; should we rename this test?

[giladchase]

Reviewable status: 5 of 38 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, @dorimedini-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 3 at r3 (raw file):

Previously, dorimedini-starkware wrote…

with OnceLock don't I have to set the value in each test?

ya 😅 buuut looks like LazyLock was merged last month into stable, so you can just replace Lazy with std::sync::LazyLock without any further changes 😬

---

workspace_tests/version_integrity_test.rs line 71 at r3 (raw file):

Previously, dorimedini-starkware wrote…

this test checks that all path-deps are members; should we rename this test?

I think so ya, if you want

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.835 ms 65.920 ms 66.020 ms]
change: [-8.6887% -5.2724% -2.2209%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe

[github-actions]

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [48.056 ms 48.181 ms 48.355 ms]
change: [+2.2574% +2.5714% +2.9821%] (p = 0.00 < 0.05)
Performance has regressed.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [65.247 ms 65.301 ms 65.371 ms]
change: [-7.7282% -4.2388% -1.2865%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe

[giladchase]

Reviewable status: 4 of 40 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry, @dan-starkware, @dorimedini-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 62 at r6 (raw file):

#[test]
fn test_path_dependencies_are_members() {

According to this, path dependencies are automatically members:

All path dependencies residing in the workspace directory automatically become members. Additional members can be listed with the members key, which should be an array of strings containing directories with Cargo.toml files.

Does that mean that this test cannot fail?

(That's not to say that I like this workspace behavior from cargo, but I just wanna be sure the test isn't a tautology.)

Code quote:

fn test_path_dependencies_are_members() {

[dorimedini-starkware]

Reviewable status: 4 of 40 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry, @dan-starkware, @elintul, @giladchase, @Itay-Tsabary-Starkware, @liorgold2, @nadin-Starkware, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 62 at r6 (raw file):

Previously, giladchase wrote…

According to this, path dependencies are automatically members:

All path dependencies residing in the workspace directory automatically become members. Additional members can be listed with the members key, which should be an array of strings containing directories with Cargo.toml files.

Does that mean that this test cannot fail?

(That's not to say that I like this workspace behavior from cargo, but I just wanna be sure the test isn't a tautology.)

this tests that the membership is explicit, which is worth it IMO

[giladchase]

Reviewed 34 of 38 files at r1, 6 of 6 files at r8.
Reviewable status: 36 of 40 files reviewed, all discussions resolved (waiting on @dafnamatsry, @dan-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @noaov1, and @Yoni-Starkware)

---

workspace_tests/version_integrity_test.rs line 62 at r6 (raw file):

Previously, dorimedini-starkware wrote…

this tests that the membership is explicit, which is worth it IMO

Got it, as long as the link above doesn't imply that this test can never fail (that is that path deps are dynamically added to ROOT_TOML.workspace.members which sounds like an unreasonable thing to do) it's a great test.

[dorimedini-starkware]

Reviewed 4 of 4 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry, @dan-starkware, @elintul, @Itay-Tsabary-Starkware, @liorgold2, @noaov1, and @Yoni-Starkware)