[Feature] [Package System] Add option to override standard library version #13318

fEst1ck · 2024-05-17T16:54:47Z

Description

Implements a --override-std mainnet/testnet/dev option for overriding the version of standard library (aptos framework, aptos stdlib, move stdlib) as dependencies. For example, if --override-std mainnet is specified, a dependency

[dependencies]
AptosFramework = { ... }

will effectively be overridden by the one from the well-know location for stdlib with the specified version

[dependency]
AptosFramework = { git = "https://github.com/aptos-labs/aptos-core.git", rev = "mainnet", subdir = "aptos-move/framework/aptos-framework" }

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Two new tests third_pary/move/tools/move-package/tests/test_sources/compilation/std-lib-{conflicts, override} one showing the problem, one showing the solution.

trunk-io · 2024-05-17T16:54:50Z

⏱️ 2h 16m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	36m	🟩 ⬜ 🟩
rust-targeted-unit-tests	32m	🟥 ⬜ 🟩
rust-move-tests	29m	🟩 ⬜ 🟥
rust-lints	15m	🟥 ⬜ 🟩
run-tests-main-branch	13m	🟩 🟩 🟩
general-lints	6m	🟩 🟩 🟩
check-dynamic-deps	4m	🟩 🟩 🟩
semgrep/ci	1m	🟩 🟩 🟩
file_change_determinator	31s	🟩 🟩 🟩
file_change_determinator	30s	🟩 🟩 🟩
permission-check	9s	🟩 🟩 🟩
permission-check	7s	🟩 🟩 🟩
permission-check	7s	🟩 🟩 🟩
permission-check	6s	🟩 🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

wrwg · 2024-05-28T00:36:31Z

third_party/move/tools/move-package/src/resolution/resolution_graph.rs

+            } else {
+                bail!(
+                    "Invalid verion for standard libraries to override: {}",
+                    version_str


Could we say here the expected strings? See also my remark about ValueEnum derive below.

wrwg · 2024-05-28T00:49:39Z

third_party/move/tools/move-package/src/source_package/std_lib.rs

+}
+
+/// Represents a standard library version.
+pub enum StdVersion {


You could use #[derive(ValueEnum)] here and then use that type directly in the clap fields instead of strings. I believe this will give an error message about what are the allowed variants.

wrwg · 2024-05-28T00:50:58Z

third_party/move/tools/move-package/src/resolution/resolution_graph.rs

+                Some(version)
+            } else {
+                bail!(
+                    "Invalid verion for standard libraries to override: {}",


s/verion/version

no longer needed

wrwg · 2024-05-28T00:53:29Z

third_party/move/tools/move-package/src/source_package/std_lib.rs

+    /// Returns the dependency for the standard library with the given version.
+    pub fn dependency(&self, version: &StdVersion) -> Dependency {
+        let local =
+            PathBuf::from(MOVE_HOME.clone()).join(format!("{}_{}", self.as_str(), version.rev()));


Is this the same location which is chosen if someone explicitly uses the github dep in their Move.toml? I'm worried that we are potentially double caching the package here. The local path should be the same to avoid this.

vgao1996

(1/2)

Sorry for the delay. Looks great to me so far!

Let me play with the code locally and see if it handles certain details correctly (e.g. the local path).

vgao1996 · 2024-05-30T04:34:51Z

third_party/move/tools/move-package/src/lib.rs

@@ -109,6 +109,10 @@ pub struct BuildConfig {
    #[clap(name = "test-mode", long = "test", global = true)]
    pub test_mode: bool,

+    /// Whether to override the standard library with the given version.
+    #[clap(long = "override-std", global = true, value_parser)]
+    pub override_std: Option<String>,


Yeah as @wrwg mentioned, you could change this to pub override_std: Option<StdVersion> if you implement the required traits for StdVersion.

vgao1996 · 2024-05-30T04:48:35Z

third_party/move/tools/move-package/tests/test_sources/compilation/std-lib-override/Move.toml

+subdir = "aptos-move/framework/aptos-framework"
+
+[dependencies.B]
+local = "./deps_only"


Nit: the directory name "deps_only" seems a bit confusing to me. Wonder if it would be better to rename this to "intermediate_dep".

The test runner checks for directories containing "deps_only", and doesn't build the packages under that directory.

vgao1996 · 2024-05-30T04:53:02Z

third_party/move/tools/move-package/src/source_package/std_lib.rs

+            subst: None,
+            version: None,
+            digest: None,
+            git_info: Some(GitInfo {


@wrwg Just to double check: what is the local caching behavior of the git dependencies? Do we update every time the build command is run, or it has to be triggered by some other events?

Specifically I want to understand what's going to happen when we update the branches in the repo.

It depends on whether you set the skip_fetch_latest_git_deps command line argument. If we set it and the local cache exists, we will update the local cache. By default, we don't update the cache and only download if a local copy does not exist. This is implemented in download_and_update_if_remote in resolution_graph.rs.

I think its a bit different, skip will actually skip checks, and otherwise we do update. The code is quite convoluted here. https://github.com/aptos-labs/aptos-core/blob/main/third_party/move/tools/move-package/src/resolution/resolution_graph.rs#L558

fEst1ck · 2024-06-05T08:41:54Z

All comments addressed. PTAL @wrwg @vgao1996

vgao1996

Tried the tool. Seemed to work well. Good job on the work!

github-actions · 2024-06-12T04:04:49Z

✅ Forge suite `compat` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `102ad37137a737d4605c0de885d4a9418d1f5197`

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> 102ad37137a737d4605c0de885d4a9418d1f5197 (PR)
1. Check liveness of validators at old version: a68e71c05caebf01504d4499110f3fba213fb53d
compatibility::simple-validator-upgrade::liveness-check : committed: 9604.839707762943 txn/s, latency: 3440.395897622073 ms, (p50: 2600 ms, p90: 6000 ms, p99: 25200 ms), latency samples: 330540
2. Upgrading first Validator to new version: 102ad37137a737d4605c0de885d4a9418d1f5197
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1190.6345177417654 txn/s, latency: 19826.48739424704 ms, (p50: 21100 ms, p90: 35200 ms, p99: 38300 ms), latency samples: 59100
3. Upgrading rest of first batch to new version: 102ad37137a737d4605c0de885d4a9418d1f5197
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7053.601010095194 txn/s, latency: 4221.626500934276 ms, (p50: 4100 ms, p90: 6100 ms, p99: 6500 ms), latency samples: 246180
4. upgrading second batch to new version: 102ad37137a737d4605c0de885d4a9418d1f5197
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11191.77146612375 txn/s, latency: 2856.9598209939973 ms, (p50: 2700 ms, p90: 5100 ms, p99: 6500 ms), latency samples: 369820
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> 102ad37137a737d4605c0de885d4a9418d1f5197 passed
Test Ok

github-actions · 2024-06-12T04:06:43Z

✅ Forge suite `realistic_env_max_load` success on `102ad37137a737d4605c0de885d4a9418d1f5197`

two traffics test: inner traffic : committed: 9466.722947114686 txn/s, latency: 4151.551665051537 ms, (p50: 4100 ms, p90: 4500 ms, p99: 5700 ms), latency samples: 4086360
two traffics test : committed: 100.07957906095359 txn/s, latency: 2050.9255813953487 ms, (p50: 1900 ms, p90: 2100 ms, p99: 6700 ms), latency samples: 1720
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.233, avg: 0.215", "QsPosToProposal: max: 1.973, avg: 1.880", "ConsensusProposalToOrdered: max: 0.312, avg: 0.283", "ConsensusOrderedToCommit: max: 0.370, avg: 0.356", "ConsensusProposalToCommit: max: 0.651, avg: 0.639"]
Max round gap was 1 [limit 4] at version 1969345. Max no progress secs was 5.44278 [limit 15] at version 1969345.
Test Ok

github-actions · 2024-06-12T04:08:19Z

✅ Forge suite `framework_upgrade` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `102ad37137a737d4605c0de885d4a9418d1f5197`

Compatibility test results for a68e71c05caebf01504d4499110f3fba213fb53d ==> 102ad37137a737d4605c0de885d4a9418d1f5197 (PR)
Upgrade the nodes to version: 102ad37137a737d4605c0de885d4a9418d1f5197
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1207.1423830371305 txn/s, submitted: 1209.6229670486023 txn/s, failed submission: 2.4805840114717794 txn/s, expired: 2.4805840114717794 txn/s, latency: 2681.19278909023 ms, (p50: 1800 ms, p90: 4800 ms, p99: 12300 ms), latency samples: 107060
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1197.073613175412 txn/s, submitted: 1198.023294740803 txn/s, failed submission: 0.9496815653910449 txn/s, expired: 0.9496815653910449 txn/s, latency: 2667.077905593019 ms, (p50: 2100 ms, p90: 4200 ms, p99: 8100 ms), latency samples: 100840
5. check swarm health
Compatibility test for a68e71c05caebf01504d4499110f3fba213fb53d ==> 102ad37137a737d4605c0de885d4a9418d1f5197 passed
Upgrade the remaining nodes to version: 102ad37137a737d4605c0de885d4a9418d1f5197
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1163.324780666772 txn/s, submitted: 1166.0370202952938 txn/s, failed submission: 2.7122396285217145 txn/s, expired: 2.7122396285217145 txn/s, latency: 2606.9580046629103 ms, (p50: 2100 ms, p90: 4500 ms, p99: 9100 ms), latency samples: 102940
Test Ok

fEst1ck force-pushed the duplicate-std branch from 41647a1 to 3bc6e61 Compare May 23, 2024 06:34

fEst1ck marked this pull request as ready for review May 23, 2024 06:43

fEst1ck requested review from gregnazario, banool, davidiw, movekevin and wrwg as code owners May 23, 2024 06:43

fEst1ck requested review from vgao1996 and removed request for davidiw, gregnazario, banool and movekevin May 23, 2024 18:14

wrwg reviewed May 28, 2024

View reviewed changes

vgao1996 reviewed May 30, 2024

View reviewed changes

fEst1ck force-pushed the duplicate-std branch from 376e846 to 5553f5b Compare June 5, 2024 08:41

fEst1ck requested review from vgao1996 and wrwg June 5, 2024 08:43

wrwg approved these changes Jun 10, 2024

View reviewed changes

vgao1996 approved these changes Jun 11, 2024

View reviewed changes

fEst1ck enabled auto-merge (squash) June 12, 2024 00:33

This comment has been minimized.

Sign in to view

Zekun Wang added 2 commits June 11, 2024 22:06

add override-std option

43a69eb

impl stdlib override

a1e60f4

Zekun Wang added 8 commits June 11, 2024 22:06

add command line arg and fix bug

de699ac

update tests

e8f70da

typo

0c63711

bug fix

0443631

update tests

8c61238

format

0985d83

refactor

96479d3

format

7b54708

fEst1ck force-pushed the duplicate-std branch from 1ac0e47 to 7b54708 Compare June 12, 2024 02:11

This comment has been minimized.

Sign in to view

update test

3c6d6b0

This comment has been minimized.

Sign in to view

Merge branch 'main' into duplicate-std

102ad37

This comment has been minimized.

Sign in to view

fEst1ck merged commit bbce0f1 into aptos-labs:main Jun 12, 2024
51 checks passed

[Feature] [Package System] Add option to override standard library version #13318

[Feature] [Package System] Add option to override standard library version #13318

Conversation

fEst1ck commented May 17, 2024 • edited

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

trunk-io bot commented May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fEst1ck Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fEst1ck commented Jun 5, 2024

vgao1996 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jun 12, 2024

✅ Forge suite compat success on a68e71c05caebf01504d4499110f3fba213fb53d ==> 102ad37137a737d4605c0de885d4a9418d1f5197

github-actions bot commented Jun 12, 2024

✅ Forge suite realistic_env_max_load success on 102ad37137a737d4605c0de885d4a9418d1f5197

github-actions bot commented Jun 12, 2024

✅ Forge suite framework_upgrade success on a68e71c05caebf01504d4499110f3fba213fb53d ==> 102ad37137a737d4605c0de885d4a9418d1f5197

fEst1ck commented May 17, 2024 •

edited

trunk-io bot commented May 17, 2024 •

edited

fEst1ck Jun 5, 2024 •

edited

✅ Forge suite `compat` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `102ad37137a737d4605c0de885d4a9418d1f5197`

✅ Forge suite `realistic_env_max_load` success on `102ad37137a737d4605c0de885d4a9418d1f5197`

✅ Forge suite `framework_upgrade` success on `a68e71c05caebf01504d4499110f3fba213fb53d` ==> `102ad37137a737d4605c0de885d4a9418d1f5197`