Customize statis initialize move dir in simtest #18752
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@halfprice is attempting to deploy a commit to the Mysten Labs Team on Vercel. A member of the Team first needs to authorize it. |
halfprice
force-pushed
the
zhewu/simtest-custom-move-dir
branch
from
2bc9ae6
to
1325d3d
Compare
halfprice
changed the title
There was a problem hiding this comment.
Thanks a lot for the PR, this allows us to clean up the Walrus repo a bit. 😁
I will defer to @mystenmark for the final approval as I haven't really been involved in this repo before.
crates/sui-proc-macros/src/lib.rs
Outdated
| let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| path.extend(["..", "..", "examples", "move", "basics"]); | ||
| path.push(env!("SIMTEST_STATIC_INIT_MOVE")); |
There was a problem hiding this comment.
It feels a bit unintuitive to have the SIMTEST_STATIC_INIT_MOVE as a relative path although it's probably useful because it allows storing the files somewhere in the repo.
Can we make this clear from the variable name? For example, SIMTEST_STATIC_INIT_MOVE_RELATIVE_PATH, although that's really a mouthful... 🤷
Also, we could do something like env::var("...").unwrap_or("../../examples/move/basics"), then we don't have to set this explicitly in this repo.
There was a problem hiding this comment.
Thanks @mlegner and they all make sense! Seems taking Mark's suggestion to use absolute path is more intuitive. I didn't find a way to get the project root dir in rust, so right now I still set it in the script. It's also easy to test so that when we set it in walrus, we know it should work.
scripts/simtest/cargo-simtest
Outdated
| @@ -96,6 +96,10 @@ fi | |||
| # Must supply a new temp dir - the test is deterministic and can't choose one randomly itself. | |||
| export TMPDIR=$(mktemp -d) | |||
|
|
|||
| # Set the example move package for the simtest static initializer | |||
| # https://github.com/MystenLabs/sui/blob/7bc276d534c6c758ac2cfefe96431c2b1318ca01/crates/sui-proc-macros/src/lib.rs#L52 | |||
| export SIMTEST_STATIC_INIT_MOVE="../../examples/move/basics" | |||
There was a problem hiding this comment.
I would recommend using git rev-parse --show-toplevel to get the repo root, and then making this an absolute path
| root_dir=$(git rev-parse --show-toplevel) | ||
| export SIMTEST_STATIC_INIT_MOVE=$root_dir"/examples/move/basics" |
There was a problem hiding this comment.
Suggestion: Put these into quotes to prevent potential issues with white spaces, etc.
| root_dir=$(git rev-parse --show-toplevel) | |
| export SIMTEST_STATIC_INIT_MOVE=$root_dir"/examples/move/basics" | |
| root_dir="$(git rev-parse --show-toplevel)" | |
| export SIMTEST_STATIC_INIT_MOVE="$root_dir/examples/move/basics" |
## Description So that other projects can choose their own move package for this static initialization. ## Test plan How did you test the new or updated feature? --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
## Description Set `SIMTEST_STATIC_INIT_MOVE` variable for Code Coverage due to #18752 ## Test plan https://github.com/MystenLabs/sui/actions/runs/10406180594/job/28818615076
…#18998) ## Description Set `SIMTEST_STATIC_INIT_MOVE` variable for Code Coverage due to MystenLabs#18752 ## Test plan https://github.com/MystenLabs/sui/actions/runs/10406180594/job/28818615076
## Description So that other projects can choose their own move package for this static initialization. ## Test plan How did you test the new or updated feature? --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
## Description Set `SIMTEST_STATIC_INIT_MOVE` variable for Code Coverage due to #18752 ## Test plan https://github.com/MystenLabs/sui/actions/runs/10406180594/job/28818615076
Description
So that other projects can choose their own move package for this static initialization.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.