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

mpm support load stdlib dynamically #3760

Merged
merged 3 commits into from
Sep 27, 2022

Conversation

pause125
Copy link
Collaborator

@pause125 pause125 commented Sep 26, 2022

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

  • mpm 集成测试时,如果当前项目是 starcoin-framework, 可以指定 --current-as-stdlib 选项,将当前项目作为 stdlib 覆盖 mpm 原本的 stdlib。mpm integration-test --ub --current-as-stdlib

Other information

starcoin-framework master 上测试时,遇到三个失败的测试用例:

  1. starcoin_upgrade_modulestarcoin_onchain_config 是因为修改了 StarcoinDAO 的 proposal config,更新测试代码即可;
  2. dao_upgrade_incompatible 错误原因不明,exp 文件输出如下:
task 16 'deploy'. lines 178-178:
Publish failure: MiscellaneousError

task 17 'run'. lines 180-188:
{
  "gas_used": 600,
  "status": "MiscellaneousError"
}

这里始终没想明白原因, @jolestar @WGB5445 请帮忙看一看。

@pause125 pause125 requested review from nanne007 and WGB5445 and removed request for nanne007 September 26, 2022 14:57
@jolestar
Copy link
Member

2 是因为 #3755 PR 里禁用了不兼容升级

Copy link
Member

@jolestar jolestar left a comment

Choose a reason for hiding this comment

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

👍,太需要这个特性了


#[clap(long)]
/// If current project is the framework project, load the modules as stdlib.
load_stdlib: bool,
Copy link
Member

Choose a reason for hiding this comment

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

感觉这个名字容易有歧义,别人会以为是指定这个参数才会加载 stdlib,否则就不加载。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reload_stdlib? refresh_stdlib? 获取其他?

Copy link
Member

Choose a reason for hiding this comment

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

干脆就直白一些? current_as_framework? use the current project as framework to build the genesis transaction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

和 current as stdlib 相比呢?stdlib 这个术语意义似乎更明确一点

Copy link
Member

Choose a reason for hiding this comment

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

current as stdlib 感觉也可以,或者干脆自动判断一下?如果当前 project 的地址是 0x1, 并且有 Genesis.move 这个 module,就自动作为 stdlib 😅。

Copy link
Member

Choose a reason for hiding this comment

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

按道理,第三方升级 mpm 也不应该 break 集成测试,因为它的依赖里已经指定了 framework 的版本。可以先合并了,先 framework 开发使用,后面给第三方用的选项如何指定,可以另外提一个 PR。 @lerencao

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

主要编译期的依赖是在 Move.toml 里配置的,和链上的依赖是独立开来的。 这个不太好控制。而且 Move.toml 里只有 commit, 没有版本号。
我想到的一个方案是 mpm 记录其所依赖的 framework commit,然后和编译期的 commit 比对,强制要求一致。
但是 commit 粒度太细了, 最好能有版本号来比对。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

其他 project 依赖 framework 会生成,但不确定是否包含在 pre_compiled_deps 中? 如果包含的话,我感觉新版的 mpm 集成测试可能会 break 第三方项目的 test 环境测试。因为新版的 mpm 的 genesis 已经更新,但第三方项目依赖的 starcoin-framework 还是旧版本,这样相当于 genesis 生成后,再用 pre_compiled_deps 中旧版本的 framework module 覆盖掉,可能会导致兼容性问题。

是不是集成测试也需要指定 starcoin-framework 版本?第三方项目必须确保集成测试中指定的版本,依赖中定义的版本一样。

project 的依赖会完整的包含在 pre_compiled_deps 中,但我觉得这不会 break 项目的测试环境。项目依赖的旧版本 framework 会覆盖 genessis 的新版本 stdlib,这只会影响 stdlib 里新增的 module, 而这些新增的 module ,第三方项目是没有使用的。可能存在兼容性问题的地方是,链下编译测试用的 framework 和 链上运行的版本不一致导致兼容性问题; 这可以要求第三方及时更新 framework。

Copy link
Member

Choose a reason for hiding this comment

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

对,旧版本 framework 会覆盖 genessis 的新版本 stdlib,但如果初始化的 resource 有改变,比如这次 DAO 的升级,可能导致旧的 module 不可用。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

啊,这确实没考虑到 resource 的变化。

@pause125
Copy link
Collaborator Author

2 是因为 #3755 PR 里禁用了不兼容升级

啊...头都要抓破了也没想明白,明明之前测试都没问题,现在咋就有问题了。。

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Merging #3760 (0c1cd26) into master (3884c4b) will increase coverage by 0.03%.
The diff coverage is 14.90%.

❗ Current head 0c1cd26 differs from pull request most recent head 4ab1aed. Consider uploading reports for the commit 4ab1aed to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3760      +/-   ##
==========================================
+ Coverage   53.69%   53.71%   +0.03%     
==========================================
  Files         594      594              
  Lines       64116    64159      +43     
==========================================
+ Hits        34418    34454      +36     
- Misses      29698    29705       +7     
Flag Coverage Δ
unittests 53.71% <14.90%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
vm/move-package-manager/src/lib.rs 0.52% <0.00%> (-<0.01%) ⬇️
...starcoin-transactional-test-harness/src/context.rs 0.00% <0.00%> (ø)
vm/starcoin-transactional-test-harness/src/lib.rs 0.11% <0.00%> (-<0.01%) ⬇️
vm/transaction-builder/src/lib.rs 32.53% <0.00%> (-0.35%) ⬇️
genesis/src/lib.rs 81.82% <46.16%> (-1.36%) ⬇️
vm/stdlib/src/lib.rs 47.85% <100.00%> (ø)
types/src/sync_status.rs 65.08% <0.00%> (-12.69%) ⬇️
...mons/forkable-jellyfish-merkle/src/iterator/mod.rs 69.79% <0.00%> (-6.11%) ⬇️
storage/src/transaction_info/mod.rs 78.58% <0.00%> (-3.57%) ⬇️
commons/service-registry/src/service_actor.rs 71.91% <0.00%> (-2.47%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d66098...4ab1aed. Read the comment docs.

@nanne007
Copy link
Member

建议用 --no-stdlib 表示不加载默认的stdlib。

@jolestar
Copy link
Member

建议用 --no-stdlib 表示不加载默认的stdlib。

但 --no-stdlib 和把当前项目作为 genesis 还是有区别的。原来也想过初始化一个空的 genesis,然后再部署,但这样 genesis 的参数不好传递。

@jolestar jolestar mentioned this pull request Sep 27, 2022
7 tasks
@jolestar jolestar enabled auto-merge (squash) September 27, 2022 02:08
@jolestar jolestar merged commit f167fe3 into starcoinorg:master Sep 27, 2022
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.

3 participants