-
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
[CI][Compiler-v2] add MOVE_LANGUAGE_V2=true
to move_pr.sh
#15309
Conversation
⏱️ 2h 31m total CI duration on this PR
🚨 2 jobs on the last run were significantly faster/slower than expected
|
MOVE_LANGUAGE_V2=true
to move_pr.sh
4924f31
to
1ea1d98
Compare
MOVE_LANGUAGE_V2=true
to move_pr.shMOVE_LANGUAGE_V2=true
to move_pr.sh
MOVE_LANGUAGE_V2=true
to move_pr.shMOVE_LANGUAGE_V2=true
to move_pr.sh
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.
Left some minor comments, LGTM otherwise.
aptos-move/e2e-move-tests/src/lib.rs
Outdated
@@ -60,5 +60,10 @@ pub(crate) fn build_package_with_compiler_version( | |||
) -> anyhow::Result<BuiltPackage> { | |||
let mut options = options; | |||
options.compiler_version = Some(compiler_version); | |||
if options.compiler_version.unwrap() != CompilerVersion::V1 { |
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.
Instead of unwrapping the just now set options.compiler_version
, use compiler_version
(the param) instead?
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
aptos-move/e2e-move-tests/src/lib.rs
Outdated
@@ -60,5 +60,10 @@ pub(crate) fn build_package_with_compiler_version( | |||
) -> anyhow::Result<BuiltPackage> { | |||
let mut options = options; | |||
options.compiler_version = Some(compiler_version); | |||
if options.compiler_version.unwrap() != CompilerVersion::V1 { | |||
options.language_version = Some(move_model::metadata::LanguageVersion::latest_stable()); |
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.
importing move_model::...::LanguageVersion
might be better here.
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
aptos-move/e2e-move-tests/src/lib.rs
Outdated
options.language_version = Some(move_model::metadata::LanguageVersion::latest_stable()); | ||
} else { | ||
options.language_version = Some(move_model::metadata::LanguageVersion::V1); | ||
} |
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.
nit: we may want to add something like infer_language_version(compiler_version: CompilerVersion)
if this pattern is going to show up regularly (which it might, similarly to inferring bytecode version).
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
1ea1d98
to
bab7467
Compare
bab7467
to
ebca39a
Compare
ebca39a
to
5ffa738
Compare
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
Add
MOVE_LANGUAGE_V2=true
for compiler v2 integration tests to allow V2 language features to be implemented in aptos-frameworkHow Has This Been Tested?
existing tests
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist