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

use feature branch for cadence->flow-go sync PRs #1441

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

robert-e-davidson3
Copy link
Contributor

@robert-e-davidson3 robert-e-davidson3 commented Feb 23, 2022

Creates feature-secure-cadence branch off of master, if needed.

Closes #1415

Description

We want to keep the flow-go and cadence repos synchronized. But that does not always extend to their master branches.

To this end every cadence PR that makes it into master creates a PR in the flow-go repo. They use a version of cadence that corresponds to the cadence PR that made it into master.

Currently we base off of the flow-go master branch. This is a problem because our cadence and flow-go releases are disjoint while cadence is undergoing major breaking changes in preparation for the secure cadence release. We don't want to add such breaking changes to flow-go releases in the meantime.

The solution here is to branch off of master into a new branch feature-secure-cadence. This branch needs to be periodically (manually) synchronized with the flow-go master branch.

When it's time to release secure cadence, the feature branch will be merged into master.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

Creates feature-secure-cadence branch off of master, if needed.
@robert-e-davidson3
Copy link
Contributor Author

QUESTION: How do I test this?

  1. Is there an easy way?
  2. Do I need to spin up my own set of repos and branches?
  3. Or do I just merge to master and clean up any messes bugs create?

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

@github-actions
Copy link

github-actions bot commented Feb 23, 2022

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit bbb0c74
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Results

old.txtnew.txt
time/opdelta
NewInterpreter/new_interpreter-21.22µs ± 2%1.26µs ± 4%+2.66%(p=0.017 n=7+7)
NewInterpreter/new_sub-interpreter-22.41µs ± 1%2.45µs ± 3%+1.50%(p=0.028 n=6+6)
RuntimeFungibleTokenTransfer-21.43ms ±26%1.27ms ±29%~(p=0.383 n=7+7)
RuntimeResourceDictionaryValues-215.6ms ± 3%15.5ms ± 3%~(p=0.902 n=7+7)
ParseInfix-29.48µs ± 1%9.48µs ± 1%~(p=0.836 n=7+6)
ParseArray-215.2ms ± 7%15.4ms ±10%~(p=1.000 n=6+7)
ParseDeploy/byte_array-224.4ms ±16%25.2ms ±11%~(p=0.535 n=7+7)
ParseDeploy/decode_hex-21.35ms ± 3%1.35ms ± 3%~(p=0.902 n=7+7)
ParseFungibleToken-2210µs ± 2%209µs ± 2%~(p=0.366 n=7+6)
QualifiedIdentifierCreation/One_level-22.69ns ± 1%2.72ns ± 3%~(p=0.129 n=6+7)
QualifiedIdentifierCreation/Three_levels-2162ns ± 3%160ns ± 1%~(p=0.469 n=7+6)
ContractInterfaceFungibleToken-244.8µs ± 1%45.2µs ± 2%~(p=0.240 n=6+6)
CheckContractInterfaceFungibleTokenConformance-2148µs ± 2%150µs ± 4%~(p=0.259 n=7+7)
InterpretRecursionFib-22.66ms ± 1%2.65ms ± 3%~(p=0.945 n=6+7)
 
alloc/opdelta
QualifiedIdentifierCreation/One_level-20.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-264.0B ± 0%64.0B ± 0%~(all equal)
ContractInterfaceFungibleToken-226.6kB ± 0%26.6kB ± 0%~(p=0.245 n=7+6)
CheckContractInterfaceFungibleTokenConformance-266.2kB ± 0%66.2kB ± 0%~(all equal)
NewInterpreter/new_interpreter-2768B ± 0%768B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-21.24kB ± 0%1.24kB ± 0%~(all equal)
InterpretRecursionFib-21.24MB ± 0%1.24MB ± 0%~(p=0.462 n=7+7)
RuntimeResourceDictionaryValues-24.05MB ± 0%4.05MB ± 0%−0.13%(p=0.001 n=6+7)
RuntimeFungibleTokenTransfer-2282kB ± 0%272kB ± 0%−3.54%(p=0.001 n=7+7)
 
allocs/opdelta
QualifiedIdentifierCreation/One_level-20.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-22.00 ± 0%2.00 ± 0%~(all equal)
ContractInterfaceFungibleToken-2458 ± 0%458 ± 0%~(all equal)
CheckContractInterfaceFungibleTokenConformance-21.07k ± 0%1.07k ± 0%~(all equal)
NewInterpreter/new_interpreter-212.0 ± 0%12.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-238.0 ± 0%38.0 ± 0%~(all equal)
InterpretRecursionFib-225.0k ± 0%25.0k ± 0%~(all equal)
RuntimeResourceDictionaryValues-2102k ± 0%102k ± 0%−0.02%(p=0.001 n=7+7)
RuntimeFungibleTokenTransfer-24.54k ± 0%4.52k ± 0%−0.34%(p=0.001 n=7+7)
 

@turbolent
Copy link
Member

@robert-e-davidson3 feel free to merge

branch names should be feature/ not feature-

Co-authored-by: Supun Setunga <[email protected]>
@robert-e-davidson3 robert-e-davidson3 merged commit 3787e12 into master Mar 1, 2022
@@ -45,6 +47,16 @@ jobs:
NEW_BRANCH=auto-cadence-upgrade/$( date +%s )/${{ github.event.pull_request.head.ref }}
echo "NEW_BRANCH=$NEW_BRANCH" >> $GITHUB_ENV

# create feature branch if needed
- name: Create feature-secure-cadence branch
Copy link
Member

Choose a reason for hiding this comment

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

@robert-e-davidson3 Shouldn't this match with the above branch name (feature/secure-cadence)?

# create feature branch if needed
- name: Create feature-secure-cadence branch
uses: peterjgrainger/[email protected]
if: ${{ env.BASE_BRANCH == "feature-secure-cadence" }}
Copy link
Member

Choose a reason for hiding this comment

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

same here

env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
branch: feature-secure-cadence
Copy link
Member

Choose a reason for hiding this comment

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

also here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update flow-go CI action to update feature branch
3 participants