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

Use cargo deny to prevent onboarding certain crates #13938

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

banool
Copy link
Contributor

@banool banool commented Jul 8, 2024

Description

Internal context: https://aptos-org.slack.com/archives/C03K45JU90C/p1720451868050819.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

CI is expected to fail until @grao1991 lands his PR. See thread in summary for more context. Once that is done, see that the new CI passes.

Update: CI passes now that that PR has been landed. Let's land.

Key Areas to Review

Confirm that deny.toml is configured appropriately.

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Jul 8, 2024

⏱️ 5h 1m total CI duration on this PR
Job Cumulative Duration Recent Runs
test-fuzzers 1h 49m 🟩🟩🟩
execution-performance / single-node-performance 32m 🟩
forge-framework-upgrade-test / forge 17m 🟩
forge-e2e-test / forge 16m 🟩
rust-move-tests 15m 🟩
forge-compat-test / forge 15m 🟩
rust-move-tests 15m 🟩
rust-move-tests 14m 🟩
rust-move-unit-coverage 13m 🟩
rust-move-unit-coverage 12m 🟩
rust-images / rust-all 11m 🟩
test-target-determinator 7m 🟩
execution-performance / test-target-determinator 7m 🟩
check 6m 🟩
general-lints 5m 🟩🟩🟩
rust-cargo-deny 3m 🟩🟩
check-dynamic-deps 2m 🟩🟩🟩
semgrep/ci 1m 🟩🟩🟩
file_change_determinator 31s 🟩🟩🟩
file_change_determinator 29s 🟩🟩🟩
file_change_determinator 10s 🟩
permission-check 8s 🟩🟩🟩
permission-check 7s 🟩🟩🟩
permission-check 7s 🟩🟩🟩
permission-check 6s 🟩🟩🟩
permission-check 3s 🟩
determine-docker-build-metadata 3s 🟩

🚨 5 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 32m 20m +61%
check 6m 4m +47%
test-target-determinator 7m 5m +38%
execution-performance / test-target-determinator 7m 5m +35%
forge-framework-upgrade-test / forge 17m 14m +26%

settingsfeedbackdocs ⋅ learn more about trunk.io

@banool banool marked this pull request as ready for review July 8, 2024 21:05
@banool banool requested review from alinush, rex1fernando and a team as code owners July 8, 2024 21:05
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.0%. Comparing base (5b20c16) to head (b5fb4d8).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #13938   +/-   ##
=======================================
  Coverage    59.0%    59.0%           
=======================================
  Files         821      821           
  Lines      198127   198127           
=======================================
  Hits       117008   117008           
  Misses      81119    81119           

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

@JoshLind JoshLind requested a review from a team July 8, 2024 23:42
@banool banool force-pushed the banool/cargo-deny-license branch from f02984e to b5fb4d8 Compare July 9, 2024 10:46
Copy link
Contributor

@JoshLind JoshLind left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @banool 😄

@@ -2,7 +2,7 @@
name = "aptos-abstract-gas-usage"
version = "0.1.0"
edition = "2021"
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
license = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly question: why do we need to add this entry to some toml files, but not others? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe all the other ones have license already and just these ones were missing it and therefore unlicensed according to the tool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah, nice, didn't realize they were all missing the workspace keys, e.g.,

# Workspace inherited keys
authors = { workspace = true }
edition = { workspace = true }
homepage = { workspace = true }
license = { workspace = true }
publish = { workspace = true }
repository = { workspace = true }
rust-version = { workspace = true }

@banool banool enabled auto-merge (squash) July 9, 2024 17:13

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Jul 9, 2024

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470 (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 8273.292906770821 txn/s, latency: 3345.431124260355 ms, (p50: 2700 ms, p90: 4200 ms, p99: 23800 ms), latency samples: 354900
2. Upgrading first Validator to new version: b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3009.6489163399415 txn/s, latency: 8531.209901666212 ms, (p50: 9400 ms, p90: 10600 ms, p99: 10800 ms), latency samples: 73220
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3249.588460355103 txn/s, latency: 9543.165251641138 ms, (p50: 9500 ms, p90: 14800 ms, p99: 15100 ms), latency samples: 137100
3. Upgrading rest of first batch to new version: b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3296.5584183232368 txn/s, latency: 7325.4292976371125 ms, (p50: 8500 ms, p90: 11800 ms, p99: 12900 ms), latency samples: 92260
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3349.821641418768 txn/s, latency: 9232.83769196362 ms, (p50: 9900 ms, p90: 14200 ms, p99: 15000 ms), latency samples: 134140
4. upgrading second batch to new version: b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 507.5453364824968 txn/s, submitted: 599.6164409976925 txn/s, failed submission: 58.16279477677617 txn/s, expired: 92.07110451519567 txn/s, latency: 39899.53334514738 ms, (p50: 39200 ms, p90: 44400 ms, p99: 44600 ms), latency samples: 33858
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6101.294248792867 txn/s, latency: 5235.734323295831 ms, (p50: 5000 ms, p90: 7800 ms, p99: 9300 ms), latency samples: 241760
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470 passed
Test Ok

Copy link
Contributor

github-actions bot commented Jul 9, 2024

✅ Forge suite realistic_env_max_load success on b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470

two traffics test: inner traffic : committed: 8398.492832301165 txn/s, latency: 4666.930554774545 ms, (p50: 4500 ms, p90: 5700 ms, p99: 10800 ms), latency samples: 3627780
two traffics test : committed: 100.02953375443529 txn/s, latency: 1963.7305555555556 ms, (p50: 1900 ms, p90: 2200 ms, p99: 4100 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.217, avg: 0.208", "QsPosToProposal: max: 0.192, avg: 0.180", "ConsensusProposalToOrdered: max: 0.307, avg: 0.287", "ConsensusOrderedToCommit: max: 0.364, avg: 0.355", "ConsensusProposalToCommit: max: 0.650, avg: 0.642"]
Max round gap was 1 [limit 4] at version 961692. Max no progress secs was 5.076736 [limit 15] at version 3567046.
Test Ok

Copy link
Contributor

github-actions bot commented Jul 9, 2024

✅ Forge suite framework_upgrade success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470 (PR)
Upgrade the nodes to version: b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1126.211331846953 txn/s, submitted: 1128.2377016963378 txn/s, failed submission: 2.0263698493847615 txn/s, expired: 2.0263698493847615 txn/s, latency: 2685.3509096361454 ms, (p50: 1800 ms, p90: 4800 ms, p99: 9300 ms), latency samples: 100040
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1095.416048348446 txn/s, submitted: 1098.218818621406 txn/s, failed submission: 2.802770272959777 txn/s, expired: 2.802770272959777 txn/s, latency: 2829.1806289978676 ms, (p50: 2100 ms, p90: 5000 ms, p99: 9700 ms), latency samples: 93800
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470 passed
Upgrade the remaining nodes to version: b5fb4d80cc3449e9b163ce8f825e4c0c24fe4470
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1192.2939996436012 txn/s, submitted: 1194.3747919640962 txn/s, failed submission: 2.080792320494941 txn/s, expired: 2.080792320494941 txn/s, latency: 2840.9635543920886 ms, (p50: 2100 ms, p90: 4500 ms, p99: 9600 ms), latency samples: 103140
Test Ok

@banool banool merged commit 272f45b into main Jul 9, 2024
105 checks passed
@banool banool deleted the banool/cargo-deny-license branch July 9, 2024 17:56
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.

None yet

3 participants