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

Ignore Cargo.lock files #1044

Closed

Conversation

jbesraa
Copy link
Contributor

@jbesraa jbesraa commented Jul 9, 2024

add Cargo.lock files to .gitignore

Copy link
Contributor

github-actions bot commented Jul 9, 2024

🐰Bencher

ReportMon, July 22, 2024 at 14:16:58 UTC
ProjectStratum v2 (SRI)
Branch2024-07-09-ignore-lock-files
Testbedsv1
Click to view all benchmark results
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
Estimated Cycles Upper Boundary
estimated cycles | (%)
InstructionsInstructions Results
instructions | (Δ%)
Instructions Upper Boundary
instructions | (%)
L1 AccessesL1 Accesses Results
accesses | (Δ%)
L1 Accesses Upper Boundary
accesses | (%)
L2 AccessesL2 Accesses Results
accesses | (Δ%)
L2 Accesses Upper Boundary
accesses | (%)
RAM AccessesRAM Accesses Results
accesses | (Δ%)
RAM Accesses Upper Boundary
accesses | (%)
get_authorize✅ (view plot)8,424.00 (-0.16%)8,723.99 (96.56%)✅ (view plot)3,746.00 (+0.12%)3,855.45 (97.16%)✅ (view plot)5,254.00 (+0.14%)5,402.39 (97.25%)✅ (view plot)4.00 (-48.99%)10.54 (37.96%)✅ (view plot)90.00 (-0.06%)93.98 (95.77%)
get_submit✅ (view plot)95,465.00 (-0.09%)96,133.47 (99.30%)✅ (view plot)59,439.00 (-0.06%)59,776.48 (99.44%)✅ (view plot)85,355.00 (-0.06%)85,834.90 (99.44%)✅ (view plot)55.00 (-0.19%)62.39 (88.16%)✅ (view plot)281.00 (-0.36%)287.58 (97.71%)
get_subscribe✅ (view plot)8,007.00 (+0.33%)8,272.54 (96.79%)✅ (view plot)2,841.00 (+0.37%)2,939.03 (96.66%)✅ (view plot)3,967.00 (+0.32%)4,098.24 (96.80%)✅ (view plot)17.00 (+3.64%)20.24 (83.99%)✅ (view plot)113.00 (+0.26%)116.98 (96.60%)
serialize_authorize✅ (view plot)12,177.00 (-0.28%)12,512.25 (97.32%)✅ (view plot)5,317.00 (+0.08%)5,426.45 (97.98%)✅ (view plot)7,417.00 (+0.10%)7,565.38 (98.04%)✅ (view plot)7.00 (-34.91%)13.49 (51.89%)✅ (view plot)135.00 (-0.49%)140.50 (96.09%)
serialize_deserialize_authorize✅ (view plot)24,444.00 (-0.12%)24,714.50 (98.91%)✅ (view plot)9,898.00 (-0.04%)10,027.72 (98.71%)✅ (view plot)13,964.00 (-0.04%)14,154.12 (98.66%)✅ (view plot)31.00 (-15.05%)41.63 (74.47%)✅ (view plot)295.00 (+0.04%)297.81 (99.06%)
serialize_deserialize_handle_authorize✅ (view plot)30,183.00 (+0.11%)30,342.06 (99.48%)✅ (view plot)12,101.00 (+0.04%)12,210.45 (99.10%)✅ (view plot)17,123.00 (+0.02%)17,280.34 (99.09%)✅ (view plot)57.00 (-3.19%)64.92 (87.81%)✅ (view plot)365.00 (+0.31%)366.92 (99.48%)
serialize_deserialize_handle_submit✅ (view plot)126,330.00 (-0.07%)127,026.22 (99.45%)✅ (view plot)73,224.00 (-0.04%)73,617.32 (99.47%)✅ (view plot)104,950.00 (-0.04%)105,509.17 (99.47%)✅ (view plot)118.00 (-1.98%)131.06 (90.04%)✅ (view plot)594.00 (-0.14%)599.19 (99.13%)
serialize_deserialize_handle_subscribe✅ (view plot)27,477.00 (+0.07%)27,604.54 (99.54%)✅ (view plot)9,643.00 (+0.11%)9,741.03 (98.99%)✅ (view plot)13,637.00 (+0.10%)13,774.15 (99.00%)✅ (view plot)66.00 (-0.16%)73.97 (89.23%)✅ (view plot)386.00 (+0.05%)388.55 (99.34%)
serialize_deserialize_submit✅ (view plot)114,989.00 (-0.08%)115,654.30 (99.42%)✅ (view plot)68,001.00 (-0.09%)68,404.17 (99.41%)✅ (view plot)97,554.00 (-0.11%)98,155.53 (99.39%)✅ (view plot)71.00 (+2.30%)75.26 (94.34%)✅ (view plot)488.00 (-0.02%)492.30 (99.13%)
serialize_deserialize_subscribe✅ (view plot)22,896.00 (+0.07%)23,117.54 (99.04%)✅ (view plot)8,195.00 (+0.10%)8,297.06 (98.77%)✅ (view plot)11,541.00 (+0.09%)11,682.18 (98.79%)✅ (view plot)38.00 (-3.78%)44.05 (86.26%)✅ (view plot)319.00 (+0.12%)321.68 (99.17%)
serialize_submit✅ (view plot)99,790.00 (-0.11%)100,465.97 (99.33%)✅ (view plot)61,483.00 (-0.05%)61,825.75 (99.45%)✅ (view plot)88,200.00 (-0.06%)88,688.14 (99.45%)✅ (view plot)57.00 (+2.33%)62.15 (91.71%)✅ (view plot)323.00 (-0.52%)329.16 (98.13%)
serialize_subscribe✅ (view plot)11,336.00 (+0.03%)11,616.10 (97.59%)✅ (view plot)4,188.00 (+0.25%)4,286.03 (97.71%)✅ (view plot)5,826.00 (+0.23%)5,957.48 (97.79%)✅ (view plot)17.00 (+3.19%)19.15 (88.78%)✅ (view plot)155.00 (-0.24%)160.07 (96.83%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Jul 9, 2024

🐰Bencher

ReportMon, July 22, 2024 at 14:16:59 UTC
ProjectStratum v2 (SRI)
Branch1044/merge
Testbedsv1

🚨 1 ALERT: Threshold Boundary Limit exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
client-sv1-authorize-serialize/client-sv1-authorize-serializeLatency (nanoseconds (ns))🚨 (view plot | view alert)258.43 (+3.89%)257.08 (100.52%)

Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client-submit-serialize✅ (view plot)6,551.90 (-5.10%)7,381.44 (88.76%)
client-submit-serialize-deserialize✅ (view plot)7,452.90 (-4.75%)8,351.18 (89.24%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle✅ (view plot)7,980.80 (-4.88%)8,872.58 (89.95%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle✅ (view plot)899.86 (+0.14%)926.90 (97.08%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize✅ (view plot)715.53 (+2.63%)718.68 (99.56%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize🚨 (view plot | view alert)258.43 (+3.89%)257.08 (100.52%)
client-sv1-get-authorize/client-sv1-get-authorize✅ (view plot)158.23 (+0.76%)162.71 (97.25%)
client-sv1-get-submit✅ (view plot)6,263.80 (-6.09%)7,164.53 (87.43%)
client-sv1-get-subscribe/client-sv1-get-subscribe✅ (view plot)276.93 (-0.57%)290.98 (95.17%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle✅ (view plot)769.59 (+2.98%)777.11 (99.03%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize✅ (view plot)630.56 (+2.44%)641.26 (98.33%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize✅ (view plot)213.03 (+3.04%)219.94 (96.86%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Jul 9, 2024

🐰Bencher

ReportMon, July 22, 2024 at 14:17:03 UTC
ProjectStratum v2 (SRI)
Branch2024-07-09-ignore-lock-files
Testbedsv2

🚨 6 ALERTS: Threshold Boundary Limits exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
client_sv2_mining_message_submit_standard_serialize_deserializeInstructions (instructions)🚨 (view plot | view alert)10,585.00 (+0.38%)10,573.43 (100.11%)
client_sv2_mining_message_submit_standard_serialize_deserializeL1 Accesses (accesses)🚨 (view plot | view alert)15,395.00 (+0.34%)15,380.08 (100.10%)
client_sv2_open_channel_serialize_deserializeInstructions (instructions)🚨 (view plot | view alert)8,027.00 (+0.51%)8,015.77 (100.14%)
client_sv2_open_channel_serialize_deserializeL1 Accesses (accesses)🚨 (view plot | view alert)11,670.00 (+0.45%)11,656.36 (100.12%)
client_sv2_setup_connection_serialize_deserializeInstructions (instructions)🚨 (view plot | view alert)14,855.00 (+0.28%)14,843.28 (100.08%)
client_sv2_setup_connection_serialize_deserializeL1 Accesses (accesses)🚨 (view plot | view alert)21,818.00 (+0.29%)21,796.79 (100.10%)

Click to view all benchmark results
BenchmarkEstimated CyclesEstimated Cycles Results
estimated cycles | (Δ%)
Estimated Cycles Upper Boundary
estimated cycles | (%)
InstructionsInstructions Results
instructions | (Δ%)
Instructions Upper Boundary
instructions | (%)
L1 AccessesL1 Accesses Results
accesses | (Δ%)
L1 Accesses Upper Boundary
accesses | (%)
L2 AccessesL2 Accesses Results
accesses | (Δ%)
L2 Accesses Upper Boundary
accesses | (%)
RAM AccessesRAM Accesses Results
accesses | (Δ%)
RAM Accesses Upper Boundary
accesses | (%)
client_sv2_handle_message_common✅ (view plot)2,095.00 (+2.10%)2,130.65 (98.33%)✅ (view plot)473.00 (+0.47%)486.21 (97.28%)✅ (view plot)735.00 (+0.37%)754.97 (97.36%)✅ (view plot)6.00 (-13.18%)10.65 (56.36%)✅ (view plot)38.00 (+3.50%)38.69 (98.22%)
client_sv2_handle_message_mining✅ (view plot)8,188.00 (-0.09%)8,332.74 (98.26%)✅ (view plot)2,137.00 (+0.44%)2,171.05 (98.43%)✅ (view plot)3,163.00 (+0.56%)3,215.87 (98.36%)✅ (view plot)32.00 (-16.34%)43.82 (73.03%)✅ (view plot)139.00 (+0.13%)141.90 (97.95%)
client_sv2_mining_message_submit_standard✅ (view plot)6,289.00 (+0.23%)6,386.04 (98.48%)✅ (view plot)1,750.00 (+0.03%)1,762.95 (99.27%)✅ (view plot)2,549.00 (-0.18%)2,575.41 (98.97%)✅ (view plot)20.00 (+13.59%)22.74 (87.95%)✅ (view plot)104.00 (+0.19%)106.84 (97.34%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)14,630.00 (-0.95%)15,028.79 (97.35%)✅ (view plot)4,694.00 (+0.01%)4,706.95 (99.72%)✅ (view plot)6,755.00 (+0.01%)6,774.87 (99.71%)✅ (view plot)49.00 (+2.85%)53.65 (91.33%)✅ (view plot)218.00 (-1.90%)229.85 (94.85%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)27,500.00 (+0.08%)27,839.23 (98.78%)🚨 (view plot | view alert)10,585.00 (+0.38%)10,573.43 (100.11%)🚨 (view plot | view alert)15,395.00 (+0.34%)15,380.08 (100.10%)✅ (view plot)90.00 (+6.94%)90.97 (98.93%)✅ (view plot)333.00 (-0.52%)345.06 (96.50%)
client_sv2_open_channel✅ (view plot)4,375.00 (-2.50%)4,611.85 (94.86%)✅ (view plot)1,461.00 (+0.05%)1,474.22 (99.10%)✅ (view plot)2,160.00 (+0.33%)2,173.07 (99.40%)✅ (view plot)9.00 (-24.10%)15.26 (58.96%)✅ (view plot)62.00 (-4.62%)68.21 (90.90%)
client_sv2_open_channel_serialize✅ (view plot)14,058.00 (-1.07%)14,457.66 (97.24%)✅ (view plot)5,064.00 (+0.02%)5,077.22 (99.74%)✅ (view plot)7,323.00 (+0.07%)7,339.22 (99.78%)✅ (view plot)38.00 (+1.92%)41.93 (90.62%)✅ (view plot)187.00 (-2.40%)198.80 (94.06%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)22,655.00 (+0.06%)23,018.54 (98.42%)🚨 (view plot | view alert)8,027.00 (+0.51%)8,015.77 (100.14%)🚨 (view plot | view alert)11,670.00 (+0.45%)11,656.36 (100.12%)✅ (view plot)83.00 (+12.54%)83.03 (99.96%)✅ (view plot)302.00 (-0.80%)314.74 (95.95%)
client_sv2_setup_connection✅ (view plot)4,723.00 (+0.50%)4,766.33 (99.09%)✅ (view plot)1,502.00 (+0.05%)1,515.22 (99.13%)✅ (view plot)2,273.00 (-0.16%)2,299.16 (98.86%)✅ (view plot)14.00 (+49.33%)14.51 (96.51%)✅ (view plot)68.00 (+0.16%)69.61 (97.69%)
client_sv2_setup_connection_serialize✅ (view plot)16,138.00 (-0.74%)16,488.65 (97.87%)✅ (view plot)5,963.00 (+0.01%)5,976.22 (99.78%)✅ (view plot)8,658.00 (+0.03%)8,677.29 (99.78%)✅ (view plot)47.00 (+4.90%)49.36 (95.22%)✅ (view plot)207.00 (-1.82%)217.49 (95.18%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)35,508.00 (-0.07%)35,742.99 (99.34%)🚨 (view plot | view alert)14,855.00 (+0.28%)14,843.28 (100.08%)🚨 (view plot | view alert)21,818.00 (+0.29%)21,796.79 (100.10%)✅ (view plot)99.00 (-0.61%)112.43 (88.05%)✅ (view plot)377.00 (-0.64%)384.54 (98.04%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

github-actions bot commented Jul 9, 2024

🐰Bencher

ReportMon, July 22, 2024 at 14:16:57 UTC
ProjectStratum v2 (SRI)
Branch2024-07-09-ignore-lock-files
Testbedsv2
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client_sv2_handle_message_common✅ (view plot)44.85 (+0.65%)45.24 (99.14%)
client_sv2_handle_message_mining✅ (view plot)72.65 (-0.77%)82.65 (87.90%)
client_sv2_mining_message_submit_standard✅ (view plot)14.67 (+0.08%)14.70 (99.79%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)268.84 (+1.47%)285.15 (94.28%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)585.68 (-1.37%)627.11 (93.39%)
client_sv2_open_channel✅ (view plot)165.06 (-0.49%)173.32 (95.23%)
client_sv2_open_channel_serialize✅ (view plot)283.76 (+0.39%)293.96 (96.53%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)372.02 (-1.64%)422.96 (87.96%)
client_sv2_setup_connection✅ (view plot)159.37 (-2.86%)174.89 (91.13%)
client_sv2_setup_connection_serialize✅ (view plot)442.07 (-6.51%)507.22 (87.16%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)951.83 (-2.12%)1,042.28 (91.32%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

Copy link
Contributor

@Shourya742 Shourya742 left a comment

Choose a reason for hiding this comment

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

Strong ACK. This is much needed.

@average-gary
Copy link
Contributor

Ack.
But for consideration leaving this here: https://doc.rust-lang.org/cargo/faq.html#why-have-cargolock-in-version-control

@plebhash
Copy link
Collaborator

rebasing to get this merged

@pavlenex pavlenex added this to the 1.0.2 milestone Jul 16, 2024
@johnnyasantoss
Copy link
Contributor

I don't think this is a good idea. Lockfiles exist for a reason, and if you read the link that @marathon-gary sent, you will see why.
There are multiple dependencies on the workspace that are unlocked. Clearly, it wasn't committed along with the dependency changes.
See it in action: fd -e toml Cargo -x sh -c 'cd {//} && cargo generate-lockfile' this will run the cargo command for every subfolder with a Cargo.toml file. After running that, you should see a lot of lockfiles generated/updated on the git working tree.

  1. This can lead to flaky CI/CD pipelines, as cargo will not know the exact version to look for.
  2. Can lead to software supply chain attack (there's no shortage in those - 1 2 3 4)
  3. Will most likelly lead to runs on my machine type of issue, preventing debugging of hard problems, git bisecting to find a regression/bug, etc.
  4. impossible to have reproducible builds (not sure if this is goal)
  5. impossible auditing

@johnnyasantoss
Copy link
Contributor

Btw, we already ignore a few lockfiles which is not ideal.

Regarding inter-workspace dependency management, I think we can stop using the version+path definition on Cargo.toml files as that just leads to unwanted changes on lock files (which this is the root of the problem - imo).
For example: commit 63b8d23 updated the sv1_api version but didn't relock the other projects, leading to unwanted changes every time you checkout dev and use an IDE/LSP that checks for project correctness

@jbesraa
Copy link
Contributor Author

jbesraa commented Jul 17, 2024

I am aware of the link you shared but dont agree with you. I think its enough to have msrv check on each pr that makes sure the pr is always building on at least 1.75.

Regrding your points:
1- why flaky tests? Cargo.toml still limits the different crates versions you know. I dont see any reason why the ci would be flaky.
2- this is not necessarily about lock files but management and inspecting of new crates added to the project.
The last three points are theoretical and I dont have much to say about that.

Anyway, my feeling is you just pouring here what you read in the link but in practice things are quite different. It would be good to try and show how this is failing rather than "it might fail"..

@johnnyasantoss
Copy link
Contributor

@jbesraa

Regrding your points: 1- why flaky tests? Cargo.toml still limits the different crates versions you know. I dont see any reason why the ci would be flaky.

Cargo.toml does not limit crates versions, it does specify the range that we want. Without a lock, it will perform dependency selection every time a fetch is run, leading to flaky CI. Also, crates can be yanked from registry.
We don't use =<version> so all our ranges are caret range which means for all crates above 1.0.0 when declared on Cargo.toml as 1.0.1 are interpreted as >=1.0.1, <2.0.0 and so it will select the latest on that range.

And, in practice, if you checkout dev (5d569bb) and run fd -e toml Cargo -j1 -x sh -c 'cd {//} && cargo +1.75 fetch && cargo +1.75 build' you will see already a few range issues (internal, imagine external deps).

error: failed to select a version for the requirement `network_helpers_sv2 = "^0.1.0"`
candidate versions found which didn't match: 2.0.0
location searched: /path/to/stratum/roles/roles-utils/network-helpers
required by package `template-provider-test v0.1.0 (/path/to/stratum/examples/template-provider-test)`

On 2) I agree with you. Lockfiles are not magical we still have to inspect dependencies before adding them to the project but to ensure that we always get the version that was inspected we need a lockfile.

Don't get me wrong, I'm not saying we can't merge this. I'm just saying this is counterproductive.

@jbesraa
Copy link
Contributor Author

jbesraa commented Jul 23, 2024

I don't have strong opinion about this PR and I would prefer it being closed rather than trying to get it merged. It could be a good convince for devs and also for PR review process, but nothing crucial.

@jbesraa jbesraa closed this Jul 23, 2024
@jbesraa jbesraa reopened this Aug 7, 2024
@jbesraa
Copy link
Contributor Author

jbesraa commented Aug 7, 2024

Re-opening this as I am not able to rebase regularly on my local machine.
Each time I git rebase -i dev the rebase stops somewhere complaining about the Cargo.lock stuff. If anyone have a strong opinion about not merging this please come forward, otherwise I think we should land this as the dev experience is currently kinda shit

@plebhash
Copy link
Collaborator

plebhash commented Aug 14, 2024

I'm doing some research on this topic. Here's some findings:

protocols/Cargo.lock + benches/Cargo.lock

I believe we can safely delete and gitignore protocols/Cargo.lock + benches/Cargo.lock. All those crates are only libs, and there's no reason to keep a Cargo.lock file for libs. Some references:

roles/Cargo.lock + utils/Cargo.lock

The crates on roles and utils workspaces do generate binaries, which means it's a good engineering practice to keep their Cargo.lock on our git history.

The root cause of our issues on this specific file comes from the fact that we often bump dependency versions on Cargo.toml files without committing the same updates to the respective Cargo.lock files.

That could be fixed by:

@rrybarczyk
Copy link
Collaborator

rrybarczyk commented Aug 15, 2024

I'm doing some research on this topic. Here's some findings:

protocols/Cargo.lock + benches/Cargo.lock

I believe we can safely delete and gitignore protocols/Cargo.lock + benches/Cargo.lock. All those crates are only libs, and there's no reason to keep a Cargo.lock file for libs. Some references:

roles/Cargo.lock + utils/Cargo.lock

The crates on roles and utils workspaces do generate binaries, which means it's a good engineering practice to keep their Cargo.lock on our git history.

The root cause of our issues on this specific file comes from the fact that we often bump dependency versions on Cargo.toml files without committing the same updates to the respective Cargo.lock files.

That could be fixed by:

This looks good to me. Basically library crates should NOT have a Cargo.lock. If it is a binary, it SHOULD have the Cargo.lock.

Thinking longer term, this now gets us back to the need to separate the library from binaries in the repo itself...the roles crate really should be a library. And then in another repo, we should have the binary applications (with the roles's toml config files there).

For now, @plebhash's suggestions make sense to me.

@plebhash plebhash removed this from the 1.0.3 milestone Aug 20, 2024
@plebhash plebhash added this to the 1.1.0 milestone Aug 20, 2024
@jbesraa jbesraa closed this Aug 21, 2024
@plebhash plebhash removed this from the 1.1.0 milestone Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done ✅
Development

Successfully merging this pull request may close these issues.

7 participants