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

pre-push githook for fmt only + remove act #1039

Merged

Conversation

rrybarczyk
Copy link
Collaborator

Addresses #1038.

@rrybarczyk rrybarczyk added the ci/cd CI/CD label Jul 5, 2024
@rrybarczyk rrybarczyk self-assigned this Jul 5, 2024
@rrybarczyk rrybarczyk linked an issue Jul 5, 2024 that may be closed by this pull request
@rrybarczyk rrybarczyk requested a review from plebhash July 5, 2024 19:40
Copy link
Contributor

github-actions bot commented Jul 5, 2024

🐰Bencher

ReportWed, August 28, 2024 at 18:34:07 UTC
ProjectStratum v2 (SRI)
Branch2024-07-pre-push-bash-fmt-only
Testbedsv2

🚨 1 ALERT: Threshold Boundary Limit exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
client_sv2_handle_message_commonLatency (nanoseconds (ns))🚨 (view plot | view alert)45.53 (+2.02%)45.50 (100.07%)

Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client_sv2_handle_message_common🚨 (view plot | view alert)45.53 (+2.02%)45.50 (100.07%)
client_sv2_handle_message_mining✅ (view plot)72.72 (-0.56%)80.50 (90.34%)
client_sv2_mining_message_submit_standard✅ (view plot)14.63 (-0.13%)14.70 (99.57%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)269.44 (+1.40%)285.38 (94.42%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)582.63 (-1.76%)624.72 (93.26%)
client_sv2_open_channel✅ (view plot)162.84 (-1.58%)171.00 (95.23%)
client_sv2_open_channel_serialize✅ (view plot)273.84 (-3.14%)293.49 (93.31%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)373.39 (-1.07%)419.14 (89.09%)
client_sv2_setup_connection✅ (view plot)165.18 (+1.00%)174.18 (94.83%)
client_sv2_setup_connection_serialize✅ (view plot)468.68 (-0.97%)502.32 (93.30%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)1,020.50 (+5.02%)1,040.37 (98.09%)

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

Copy link
Contributor

github-actions bot commented Jul 5, 2024

🐰Bencher

ReportWed, August 28, 2024 at 18:34:29 UTC
ProjectStratum v2 (SRI)
Branch2024-07-pre-push-bash-fmt-only
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,541.00 (+1.18%)8,740.96 (97.71%)✅ (view plot)3,772.00 (+0.76%)3,862.39 (97.66%)✅ (view plot)5,296.00 (+0.85%)5,413.32 (97.83%)✅ (view plot)5.00 (-33.00%)10.60 (47.15%)✅ (view plot)92.00 (+2.14%)94.22 (97.64%)
get_submit✅ (view plot)95,718.00 (+0.16%)96,154.02 (99.55%)✅ (view plot)59,522.00 (+0.08%)59,767.47 (99.59%)✅ (view plot)85,498.00 (+0.09%)85,826.36 (99.62%)✅ (view plot)49.00 (-9.16%)62.86 (77.95%)✅ (view plot)285.00 (+0.99%)288.84 (98.67%)
get_subscribe✅ (view plot)8,102.00 (+1.46%)8,296.33 (97.66%)✅ (view plot)2,848.00 (+0.64%)2,944.19 (96.73%)✅ (view plot)3,982.00 (+0.68%)4,106.94 (96.96%)✅ (view plot)12.00 (-21.18%)20.99 (57.17%)✅ (view plot)116.00 (+2.67%)117.82 (98.45%)
serialize_authorize✅ (view plot)12,392.00 (+1.35%)12,556.64 (98.69%)✅ (view plot)5,343.00 (+0.54%)5,433.39 (98.34%)✅ (view plot)7,457.00 (+0.59%)7,575.79 (98.43%)✅ (view plot)7.00 (-31.43%)13.87 (50.45%)✅ (view plot)140.00 (+2.88%)141.62 (98.86%)
serialize_deserialize_authorize✅ (view plot)24,670.00 (+0.72%)24,767.17 (99.61%)✅ (view plot)9,950.00 (+0.43%)10,037.67 (99.13%)✅ (view plot)14,050.00 (+0.50%)14,170.99 (99.15%)✅ (view plot)31.00 (-13.28%)42.21 (73.43%)✅ (view plot)299.00 (+1.25%)299.10 (99.97%)
serialize_deserialize_handle_authorize✅ (view plot)30,270.00 (+0.39%)30,391.00 (99.60%)✅ (view plot)12,127.00 (+0.24%)12,217.39 (99.26%)✅ (view plot)17,165.00 (+0.24%)17,290.45 (99.27%)✅ (view plot)59.00 (+1.31%)64.43 (91.58%)✅ (view plot)366.00 (+0.56%)367.70 (99.54%)
serialize_deserialize_handle_submit✅ (view plot)126,605.00 (+0.14%)127,072.68 (99.63%)✅ (view plot)73,307.00 (+0.07%)73,609.10 (99.59%)✅ (view plot)105,095.00 (+0.09%)105,503.44 (99.61%)✅ (view plot)109.00 (-7.63%)132.69 (82.15%)✅ (view plot)599.00 (+0.62%)601.62 (99.56%)
serialize_deserialize_handle_subscribe✅ (view plot)27,614.00 (+0.53%)27,676.17 (99.78%)✅ (view plot)9,650.00 (+0.19%)9,746.19 (99.01%)✅ (view plot)13,649.00 (+0.18%)13,781.13 (99.04%)✅ (view plot)63.00 (-3.48%)73.29 (85.95%)✅ (view plot)390.00 (+0.98%)390.18 (99.95%)
serialize_deserialize_submit✅ (view plot)115,367.00 (+0.21%)115,719.41 (99.70%)✅ (view plot)68,167.00 (+0.13%)68,406.65 (99.65%)✅ (view plot)97,842.00 (+0.16%)98,169.36 (99.67%)✅ (view plot)61.00 (-10.35%)76.97 (79.26%)✅ (view plot)492.00 (+0.70%)494.53 (99.49%)
serialize_deserialize_subscribe✅ (view plot)23,088.00 (+0.82%)23,183.27 (99.59%)✅ (view plot)8,209.00 (+0.27%)8,302.84 (98.87%)✅ (view plot)11,563.00 (+0.27%)11,690.40 (98.91%)✅ (view plot)37.00 (-4.87%)44.05 (83.99%)✅ (view plot)324.00 (+1.48%)324.01 (100.00%)
serialize_submit✅ (view plot)100,125.00 (+0.20%)100,507.61 (99.62%)✅ (view plot)61,566.00 (+0.07%)61,816.09 (99.60%)✅ (view plot)88,345.00 (+0.09%)88,679.29 (99.62%)✅ (view plot)46.00 (-14.84%)64.54 (71.27%)✅ (view plot)330.00 (+1.45%)331.94 (99.42%)
serialize_subscribe✅ (view plot)11,541.00 (+1.66%)11,676.20 (98.84%)✅ (view plot)4,195.00 (+0.43%)4,291.19 (97.76%)✅ (view plot)5,836.00 (+0.40%)5,964.84 (97.84%)✅ (view plot)14.00 (-10.84%)19.83 (70.60%)✅ (view plot)161.00 (+3.18%)162.05 (99.35%)

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

Copy link
Contributor

github-actions bot commented Jul 5, 2024

🐰Bencher

ReportWed, August 28, 2024 at 18:34:21 UTC
ProjectStratum v2 (SRI)
Branch1039/merge
Testbedsv1
Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns) | (Δ%)
Latency Upper Boundary
nanoseconds (ns) | (%)
client-submit-serialize✅ (view plot)6,658.50 (-2.89%)7,359.84 (90.47%)
client-submit-serialize-deserialize✅ (view plot)7,567.20 (-2.70%)8,344.05 (90.69%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle✅ (view plot)8,162.80 (-2.30%)8,844.25 (92.29%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle✅ (view plot)892.88 (-0.79%)927.78 (96.24%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize✅ (view plot)700.49 (+0.12%)721.49 (97.09%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize✅ (view plot)250.17 (+0.58%)255.89 (97.77%)
client-sv1-get-authorize/client-sv1-get-authorize✅ (view plot)157.66 (+0.35%)161.81 (97.44%)
client-sv1-get-submit✅ (view plot)6,414.10 (-3.34%)7,125.87 (90.01%)
client-sv1-get-subscribe/client-sv1-get-subscribe✅ (view plot)276.87 (-0.63%)289.82 (95.53%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle✅ (view plot)769.95 (+2.59%)780.38 (98.66%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize✅ (view plot)614.48 (+0.02%)636.65 (96.52%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize✅ (view plot)205.90 (-0.52%)219.00 (94.02%)

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

Copy link
Contributor

github-actions bot commented Jul 5, 2024

🐰Bencher

ReportWed, August 28, 2024 at 18:34:09 UTC
ProjectStratum v2 (SRI)
Branch2024-07-pre-push-bash-fmt-only
Testbedsv2

🚨 4 ALERTS: Threshold Boundary Limits exceeded!
BenchmarkMeasure (units)ViewValueLower BoundaryUpper Boundary
client_sv2_handle_message_commonEstimated Cycles (estimated cycles)🚨 (view plot | view alert)2,191.00 (+6.39%)2,148.03 (102.00%)
client_sv2_handle_message_commonL2 Accesses (accesses)🚨 (view plot | view alert)13.00 (+74.95%)12.02 (108.12%)
client_sv2_handle_message_commonRAM Accesses (accesses)🚨 (view plot | view alert)40.00 (+8.51%)39.00 (102.56%)
client_sv2_mining_message_submit_standardEstimated Cycles (estimated cycles)🚨 (view plot | view alert)6,404.00 (+1.93%)6,396.13 (100.12%)

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 | view alert)2,191.00 (+6.39%)2,148.03 (102.00%)✅ (view plot)473.00 (+0.39%)485.63 (97.40%)✅ (view plot)726.00 (-0.84%)753.25 (96.38%)🚨 (view plot | view alert)13.00 (+74.95%)12.02 (108.12%)🚨 (view plot | view alert)40.00 (+8.51%)39.00 (102.56%)
client_sv2_handle_message_mining✅ (view plot)8,255.00 (+0.61%)8,337.54 (99.01%)✅ (view plot)2,143.00 (+0.61%)2,171.82 (98.67%)✅ (view plot)3,165.00 (+0.52%)3,215.89 (98.42%)✅ (view plot)38.00 (+0.04%)43.24 (87.87%)✅ (view plot)140.00 (+0.69%)142.02 (98.58%)
client_sv2_mining_message_submit_standard🚨 (view plot | view alert)6,404.00 (+1.93%)6,396.13 (100.12%)✅ (view plot)1,756.00 (+0.30%)1,764.32 (99.53%)✅ (view plot)2,554.00 (-0.03%)2,575.42 (99.17%)✅ (view plot)21.00 (+19.32%)22.59 (92.98%)✅ (view plot)107.00 (+2.88%)107.01 (99.99%)
client_sv2_mining_message_submit_standard_serialize✅ (view plot)14,903.00 (+0.79%)15,025.06 (99.19%)✅ (view plot)4,700.00 (+0.11%)4,708.32 (99.82%)✅ (view plot)6,758.00 (+0.03%)6,775.58 (99.74%)✅ (view plot)47.00 (+0.07%)51.38 (91.47%)✅ (view plot)226.00 (+1.46%)229.67 (98.40%)
client_sv2_mining_message_submit_standard_serialize_deserialize✅ (view plot)27,685.00 (+0.67%)27,847.48 (99.42%)✅ (view plot)10,591.00 (+0.39%)10,592.26 (99.99%)✅ (view plot)15,405.00 (+0.35%)15,410.19 (99.97%)✅ (view plot)83.00 (-0.20%)88.68 (93.59%)✅ (view plot)339.00 (+1.12%)344.88 (98.29%)
client_sv2_open_channel✅ (view plot)4,425.00 (-1.03%)4,628.49 (95.60%)✅ (view plot)1,461.00 (+0.04%)1,473.45 (99.16%)✅ (view plot)2,155.00 (+0.07%)2,172.39 (99.20%)✅ (view plot)13.00 (+7.37%)15.14 (85.85%)✅ (view plot)63.00 (-2.31%)68.91 (91.43%)
client_sv2_open_channel_serialize✅ (view plot)14,140.00 (-0.41%)14,445.55 (97.88%)✅ (view plot)5,064.00 (+0.01%)5,076.45 (99.75%)✅ (view plot)7,325.00 (+0.08%)7,339.97 (99.80%)✅ (view plot)33.00 (-9.87%)41.31 (79.89%)✅ (view plot)190.00 (-0.68%)198.36 (95.78%)
client_sv2_open_channel_serialize_deserialize✅ (view plot)22,757.00 (+0.48%)22,998.15 (98.95%)✅ (view plot)8,027.00 (+0.45%)8,029.19 (99.97%)✅ (view plot)11,667.00 (+0.37%)11,676.00 (99.92%)✅ (view plot)83.00 (+12.00%)83.36 (99.57%)✅ (view plot)305.00 (+0.20%)313.96 (97.15%)
client_sv2_setup_connection✅ (view plot)4,757.00 (+1.28%)4,762.93 (99.88%)✅ (view plot)1,502.00 (+0.04%)1,514.45 (99.18%)✅ (view plot)2,272.00 (-0.20%)2,297.51 (98.89%)✅ (view plot)14.00 (+44.44%)14.89 (94.01%)✅ (view plot)69.00 (+1.82%)69.64 (99.09%)
client_sv2_setup_connection_serialize✅ (view plot)16,240.00 (-0.11%)16,470.16 (98.60%)✅ (view plot)5,963.00 (+0.01%)5,975.45 (99.79%)✅ (view plot)8,655.00 (-0.00%)8,675.95 (99.76%)✅ (view plot)47.00 (+4.48%)49.33 (95.28%)✅ (view plot)210.00 (-0.36%)216.90 (96.82%)
client_sv2_setup_connection_serialize_deserialize✅ (view plot)35,648.00 (+0.30%)35,737.79 (99.75%)✅ (view plot)14,855.00 (+0.25%)14,857.08 (99.99%)✅ (view plot)21,813.00 (+0.24%)21,820.97 (99.96%)✅ (view plot)100.00 (+1.12%)111.48 (89.70%)✅ (view plot)381.00 (+0.37%)384.04 (99.21%)

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

did you intentionally commit these Cargo.lock files or was it a mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They were generated/updated when I ran the clippy and sv2 header check script. I was not sure if I should commit them or not. Should I remove?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I would remove them from this PR

this is probably something more relevant for #1044

act --job clippy-check --reuse -P ubuntu-latest=ghcr.io/catthehacker/ubuntu:rust-latest
act --job ci --reuse -P ubuntu-latest=ghcr.io/catthehacker/ubuntu:rust-latest
# Run clippy, test, and rustfmt on all workspaces
sh ./scripts/clippy-on-all-workspaces.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

As suggested here, I would rename the script into clippy-fmt-and-test.sh

Copy link
Collaborator

Choose a reason for hiding this comment

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

now that we removed cargo version from the toml files we should enforce cargo 1.75 in the scripts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added here, lmk if that is what you had in mind.

@rrybarczyk rrybarczyk force-pushed the 2024-07-pre-push-bash-fmt-only branch from 89db3b4 to cc4668b Compare August 9, 2024 16:02
.githooks/pre-push Outdated Show resolved Hide resolved
@rrybarczyk
Copy link
Collaborator Author

rrybarczyk commented Aug 9, 2024

@plebhash, @GitGab19, and @Fi3: This is ready for review again. Please note that when the pre-push script is ran, the following Cargo.locks get changed:

  • benches/Cargo.lock
git diff benches/Cargo.lock
diff --git a/benches/Cargo.lock b/benches/Cargo.lock
index 946b7c02..39647483 100644
--- a/benches/Cargo.lock
+++ b/benches/Cargo.lock
@@ -373,7 +373,7 @@ dependencies = [

 [[package]]
 name = "buffer_sv2"
-version = "1.0.0"
+version = "1.1.0"
 dependencies = [
  "aes-gcm",
 ]
@@ -497,7 +497,7 @@ checksum = "98cc8fbded0c607b7ba9dd60cd98df59af97e84d24e49c8557331cfc26d301ce"

 [[package]]
 name = "codec_sv2"
-version = "1.1.0"
+version = "1.2.0"
 dependencies = [
  "binary_sv2",
  "buffer_sv2",
@@ -1564,7 +1564,7 @@ checksum = "81cdd64d312baedb58e21336b31bc043b77e01cc99033ce76ef539f78e965ebc"

 [[package]]
 name = "sv1_api"
-version = "1.0.0"
+version = "1.0.1"
 dependencies = [
  "binary_sv2",
  "bitcoin_hashes 0.3.2",
  • roles/Cargo.lock
git diff roles/Cargo.lock
diff --git a/roles/Cargo.lock b/roles/Cargo.lock
index bf54dcc0..b88776bd 100644
--- a/roles/Cargo.lock
+++ b/roles/Cargo.lock
@@ -1927,7 +1927,7 @@ dependencies = [

 [[package]]
 name = "serde_sv2"
-version = "1.0.0"
+version = "1.0.1"
 dependencies = [
  "buffer_sv2",
  "serde",
@@ -2053,7 +2053,7 @@ dependencies = [

 [[package]]
 name = "sv1_api"
-version = "1.0.0"
+version = "1.0.1"
 dependencies = [
  "binary_sv2",
  "bitcoin_hashes 0.3.2",

I did not commit these changes to this repo (had them in before but remove that commit). @plebhash, would this get fixed with the #985 semver PR? Can wait until that is merged to merge this one if so.

Also, unrelated to this PR, but one thing I noticed was that a couple of the tests do not complete but instead print that they have been running for over 60 seconds. This greatly slows down the runtime of the pre-push script, which is annoying:

cargo-test-run-over-60s

Does anyone know why this is happening?

@plebhash
Copy link
Collaborator

with #1102 in mind, it might be a good idea to also run this on our githook:

cargo build --manifest-path=roles/Cargo.toml --locked

@rrybarczyk
Copy link
Collaborator Author

Will change to ready for review to be merged after #1102 and #1044 are merged.

@plebhash plebhash marked this pull request as ready for review August 23, 2024 19:37
@jbesraa
Copy link
Contributor

jbesraa commented Aug 24, 2024

Does this run automatically on a pre-push or do I need to trigger it automatically?

@plebhash
Copy link
Collaborator

plebhash commented Aug 24, 2024

Does this run automatically on a pre-push or do I need to trigger it automatically?

You need to do this to enable the pre-push:

stratum/README-DEV.md

Lines 8 to 11 in 0f0ee1e

# Enable pre-push hooks
This tells git where the githooks are located
`# git config core.hooksPath .githooks`

After you do this, your local git repo will be configured to always run this script when you do git push. In order to bypass it, you will need to add --no-verify.

If you don't do the command above, the pre-push will not be used. In other words, this is opt-in.

Also see #1140 where some info about this are being elaborated.

@rrybarczyk
Copy link
Collaborator Author

The roles/Cargo.lock is not constant and the cargo build --manifest-path=roles/Cargo.toml --locked check in the pre-push fails with:

error: the lock file /Users/rachelrybarczyk/Development/StratumV2/Dev/stratum/roles/Cargo.lock needs to be updated but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network, remove the --locked flag and use --offline instead.
+ echo 'Error: Cargo.lock file in roles crate is out of date. Please run '\''cargo update'\'' in the roles crate.'
Error: Cargo.lock file in roles crate is out of date. Please run 'cargo update' in the roles crate.
+ exit 1

This Cargo.lock file needs to be fixed before this PR can be merged.

@plebhash plebhash changed the base branch from dev to main August 28, 2024 12:23
@plebhash plebhash merged commit 1c5e464 into stratum-mining:main Aug 28, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-push git hooks errors + should be limited to formatting only
5 participants