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

[Libra] Update dependencies, point to latest code changes #1252

Open
wants to merge 12 commits into
base: v6
Choose a base branch
from

Conversation

ea-open-source
Copy link
Contributor

Motivation

This PR is one part of the process to switch Carpe build to the latest v6 development.
There will be another PR after this one to update the cargo.lock file once the updated dependecies on Move-0L is accepted and with added fix to missing multisig_address of encode_community_transfer_script_function

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Pass build.

Related PRs

#70

@@ -51,8 +50,8 @@ address DiemFramework {
const INVALID_PERCENTAGE: u64 = 010022;
/// Attempt to use a UID that is already taken
const UID_TAKEN: u64 = 010023;
/// Attempt to make a payment to a non-community-wallet
const PAYEE_NOT_COMMUNITY_WALLET: u64 = 010024;
// /// Attempt to make a payment to a non-community-wallet
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add why this is commented out and what to do with it in the future?

Choose a reason for hiding this comment

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

These are not exactly my changes but auto-gerenated changes after running make stdlib

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes sorry, did not see they were in releases folder.

fun maybe_pay_if_deadline_today(vm: &signer, state: &mut TxSchedule) acquires Freeze {
let epoch = DiemConfig::get_current_epoch();
fun maybe_pay_deadline(vm: &signer, state: &mut TxSchedule, epoch: u64) acquires Freeze {
// let epoch = DiemConfig::get_current_epoch();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Choose a reason for hiding this comment

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

Same as above

api/Cargo.toml Outdated
@@ -34,8 +34,8 @@ diem-types = { path = "../types" }
diem-workspace-hack = { version = "0.1", path = "../crates/diem-workspace-hack" }
diem-api-types = { path = "./types", package = "diem-api-types" }
storage-interface = { path = "../storage/storage-interface" }
move-core-types = { git = "https://github.com/0LNetworkCommunity/move-0L", rev = "94e1039c9fdf1472c2c7775de43135d28dafc955" }
move-resource-viewer = { git = "https://github.com/0LNetworkCommunity/move-0L", rev = "94e1039c9fdf1472c2c7775de43135d28dafc955" }
move-core-types = { git = "https://github.com/0LNetworkCommunity/move-0L", branch = "v6" }
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to use rev instead of branch. Using branch is discouraged since it requires manual cargo update which is super easy to forget, causes inconsistency and confusion. Plus, our upstreams Diem, Aptos are also using rev more than branches.

Choose a reason for hiding this comment

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

Interesting, I was under the impression that this upgrade aims to remove rev and instead use branch for easier update in the future. Any concern about build reproducibility can be addressed by a static move.lock with own rev.
I understand your concern about manual cargo update but if someone want to update the rev they would have to rerun cargo update anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you do not need cargo update if you use rev, you can test it. That's why rev is suggested in the community.

@0o-de-lally
Copy link
Collaborator

@Dat-H-Tran Also take a look at the smoke and forge testsuites, I believe they are not building

@Dat-H-Tran
Copy link

@Dat-H-Tran Also take a look at the smoke and forge testsuites, I believe they are not building

As I have pointed out during our stand up last week. This failed build is because Libra imports Move-0L that import Libra.
As the Move-0L PR has been accepted the current reason why it's failing is because Move-0L expects newer dependencies from the current v6 of Libra. This PR needs to be accepted and cargo update to be run for build to pass. There is a workaround of creating another branch of Libra repo with updated dependencies and point Move-0L to that instead or revert the branch changes and move back to specifying rev.

Please give the final words on this issue of using branch instead of rev.

@simsekgokhan
Copy link
Contributor

@Dat-H-Tran Let's use revs.

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.

None yet

4 participants