Skip to content

Run cargo update to update deps to latest version #12

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.

Sign up for GitHub

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

Jump to bottom
Merged
bkchr merged 2 commits into from
Dec 3, 2021
Merged

Run cargo update to update deps to latest version #12

bkchr merged 2 commits into from
Dec 3, 2021

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Dec 2, 2021 •
edited by TriplEight
Loading

This should fix the problem we are seeing with Cumulus companions not
building, because we have merged a non-breaking pr to Polkadot.

We first need to merge the pipeline scripts pr, tag new release
And then update both of the prs
It doesn't yet solve the "breaks code in polkadot + cumulus" case. It is doable, but the current script (all this bash 🤮) requires a rewrite for this

bkchr added 2 commits December 2, 2021 14:12
@bkchr
Run cargo update to update deps to latest version
6f52ea0 
This should fix the problem we are seeing with Cumulus companions not
building, because we have merged a non-breaking pr to Polkadot.
@bkchr
🤦
f32bbec
@gui1117
Copy link

gui1117 commented Dec 3, 2021 •
edited
Loading

but does this fix the issue where a substrate PR has both a cumulus and a polkadot companion ?

AFAICT it will still only run diener ... --substrate in the cumulus check and I would expect to run both diener .. --substrate and diener --polkadot.
Otherwise the change in the polkadot companion will not be included no ?

Sorry, what I thought is just not part of this PR

gui1117
gui1117 approved these changes Dec 3, 2021
View reviewed changes
Copy link

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

seems good to me

alvicsam
alvicsam reviewed Dec 3, 2021
View reviewed changes
check_dependent_project.sh
#
# This is for example needed if there was a pr to Substrate that only required a Polkadot companion
# and Cumulus wasn't yet updated to use the latest commit of Polkadot.
for update in $update_crates_on_default_branch; do
Copy link
Contributor

@alvicsam alvicsam Dec 3, 2021

Choose a reason for hiding this comment

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

Why it needs to be run 2 times here and in patch_and_check_dependent()?

Copy link
Member Author

@bkchr bkchr Dec 3, 2021

Choose a reason for hiding this comment

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

These are 2 different code paths.

Copy link
Contributor

@TriplEight TriplEight Dec 3, 2021

Choose a reason for hiding this comment

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

same question, but it's not a big deal to run cargo update 2 times, worst case nothing happens

Copy link
Member Author

@bkchr bkchr Dec 3, 2021

Choose a reason for hiding this comment

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

These are different code paths. Both are not called together.

@bkchr bkchr merged commit 2718955 into master Dec 3, 2021
TriplEight
TriplEight approved these changes Dec 3, 2021
View reviewed changes
check_dependent_project.sh
#
# This is for example needed if there was a pr to Substrate that only required a Polkadot companion
# and Cumulus wasn't yet updated to use the latest commit of Polkadot.
for update in $update_crates_on_default_branch; do
Copy link
Contributor

@TriplEight TriplEight Dec 3, 2021

Choose a reason for hiding this comment

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

same question, but it's not a big deal to run cargo update 2 times, worst case nothing happens

@bkchr bkchr deleted the bkchr-update-cargo branch December 3, 2021 11:02
This was referenced Dec 3, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alvicsam alvicsam alvicsam left review comments

@gui1117 gui1117 gui1117 approved these changes

+1 more reviewer

@TriplEight TriplEight TriplEight approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bkchr @gui1117 @TriplEight @alvicsam