-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[CLI] Add a CLI command to install prover dependencies #14683
Conversation
⏱️ 5h 55m total CI duration on this PR
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @rahxephon89 and the rest of your teammates on Graphite |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14683 +/- ##
=======================================
Coverage 59.8% 59.8%
=======================================
Files 853 853
Lines 207917 207923 +6
=======================================
+ Hits 124342 124360 +18
+ Misses 83575 83563 -12 ☔ View full report in Codecov by Sentry. |
b0cb61e
to
4961e5a
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @rahxephon89 and the rest of your teammates on Graphite |
838ae5b
to
6bc3ba7
Compare
6bc3ba7
to
7f7b09e
Compare
3e0770b
to
938f1c0
Compare
938f1c0
to
7054457
Compare
7054457
to
9749770
Compare
@@ -13,7 +13,7 @@ use self_update::update::ReleaseUpdate; | |||
use std::path::PathBuf; | |||
|
|||
const FORMATTER_BINARY_NAME: &str = "movefmt"; | |||
const TARGET_FORMATTER_VERSION: &str = "1.0.4"; | |||
const TARGET_FORMATTER_VERSION: &str = "1.0.5"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
piggyback the latest version of movefmt
de38ab9
to
2f4f445
Compare
install_path.to_string_lossy() | ||
); | ||
if env::var(env_var).is_err() { | ||
println!("Please use the `source` command or reboot the terminal to check whether {} is set with the correct value. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
println!("Please use the `source` command or reboot the terminal to check whether {} is set with the correct value. \ | |
eprintln!("Please use the `source` command or reboot the terminal to check whether {} is set with the correct value. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
set_env::set(env_var, install_path.to_string_lossy()) | ||
.map_err(|e| CliError::UnexpectedError(format!("Failed to set {}: {}", env_var, e)))?; | ||
println!( | ||
"Added {} to environment with value: {}.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message should make it clear that we actually added this to their profile, so it is added to the environment permanently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
.await?; | ||
println!("{}", res); | ||
|
||
#[cfg(unix)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess Windows doesn't need this tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is not supported windows in the current version, which may change in the future.
pub binary_name: String, | ||
|
||
pub exe_name: String, | ||
|
||
pub env_var: String, | ||
|
||
pub version_match_string: String, | ||
|
||
pub version_option_string: String, | ||
|
||
pub target_version: String, | ||
|
||
pub install_dir: Option<PathBuf>, | ||
|
||
pub check: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on these would be nice, it's not clear what they do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
impl BinaryUpdater for DependencyInstaller { | ||
fn check(&self) -> bool { | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use self.check
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -63,6 +66,12 @@ pub fn build_updater( | |||
None => "0.0.0", | |||
}; | |||
|
|||
println!( | |||
"install_dir:{:?}, is directory:{}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"install_dir:{:?}, is directory:{}", | |
"install_dir: {:?}, is directory: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the print statement
@@ -48,7 +48,10 @@ pub fn build_updater( | |||
let target = format!("{}-{}", arch_str, target); | |||
|
|||
let install_dir = match install_dir.clone() { | |||
Some(dir) => dir, | |||
Some(dir) => { | |||
println!("dir:{:?}", dir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
println!("dir:{:?}", dir); | |
println!("dir: {:?}", dir); |
@@ -48,7 +48,10 @@ pub fn build_updater( | |||
let target = format!("{}-{}", arch_str, target); | |||
|
|||
let install_dir = match install_dir.clone() { | |||
Some(dir) => dir, | |||
Some(dir) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested formatting improvements but these sort of look like debug prints, is it intentional that this and the one for install dir are left in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it is for debugging, removed the print statement
1ece55d
to
b017c0a
Compare
b017c0a
to
2880eb0
Compare
2880eb0
to
225aa1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for getting it to this state!
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.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
This PR adds a new command
aptos update prover-dependencies
to install the prover dependency: z3, boogie and cvc5 and sets the required environment variables. With this new command, developers don't need to clone aptos-core and rundev_setup.sh
to install prover dependencies. Once this feature is released, we will update the prover installation guide.The binaries of the prover dependency is maintained in the repo prover-dependency.
Note that CI will still use
dev_setup.sh
to install prover dependencies and the check is added to the commandto guarantee versions installed by the CLI is compatible with the requirement for the prover.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Manually tested the command on macos, linux and windows.
Key Areas to Review
Checklist