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

TrustEVM Contract CI #293

Merged
merged 58 commits into from
Feb 9, 2023
Merged

TrustEVM Contract CI #293

merged 58 commits into from
Feb 9, 2023

Conversation

kj4ezj
Copy link
Contributor

@kj4ezj kj4ezj commented Jan 18, 2023

From TrustEVM issue 242, this pull request adds a GitHub Actions workflow to build the TrustEVM contract and contract unit test(s) per the instructions in the README.md. This also includes pipeline documentation, which is linked to from the root README.md and is attached at the bottom of the build summary in each build.

Example Builds

This is a separate GitHub Actions workflow from the TrustEVM node because these builds do not depend upon each other, so they can be run in parallel and their build outcomes can be considered independently.

github-app-token-action

This workflow uses the AntelopeIO/github-app-token-action GitHub action to assume the role of a GitHub application installed to the AntelopeIO organization to clone the private submodules. It requests an API key from the GitHub app, clones everything, then the key expires. This is advantageous over a persistent API key from a GitHub service account because this does not consume a paid user seat, the "account" associated with the app cannot be logged into in the GitHub web UI, the app is scoped to exactly the permissions it needs to perform the clones for this repo and nothing more, and the API key expires very quickly so a bad actor who exfiltrates this key from the CI system will find it is not useful.

The downside is that if TrustEVM adds additional private submodules, the GitHub app must be granted permissions to these new submodules. The CI system will not work until this happens. Please reach out to me via IM in the #help-automation engineering channel to have this done.

Future Work

There are several potential avenues for potential follow-up work.

Runners

This CI system currently uses the "free" and public GitHub Actions runners. This workflow completes in five to six minutes, which is reasonable but not ideal. It may complete much faster running on our own VMs but this comes at the expense of effort to implement this and cloud compute costs. Another thing is the node build took ~80 minutes on the public runners. If we put that on private runners, it may still take longer than six minutes. In that case, there is no point paying to run these builds.

In any case, this can be done in a future pull request.

Tests

The tests do not pass for me and I was informed on 2023-01-12 they are not expected to pass, so they are built but not run by CI.

Zach
This feels like a dumb question but....are the contract unit tests expected to pass on the HEAD of main (8561800)?

Matias
The short answer is "no". There is only one cpp test case that will fail if any of the eth consensus test fails and not all of them are passing at this point

If the TrustEVM contract unit test(s) end up in a good state and the team wants me to add them to CI, please open a new follow-up issue and shoot me a link via IM.

See Also

@kj4ezj kj4ezj added the CICD Continuous integration, continuous deployment - build and test system label Jan 18, 2023
@kj4ezj kj4ezj mentioned this pull request Jan 25, 2023
@kj4ezj kj4ezj marked this pull request as ready for review February 2, 2023 06:53
@spoonincode
Copy link
Member

Yeah, I do wonder if we should go ahead and let it run to failure now, just so we have a history. Or even unit_test || true if you don't like the red X.

ee mkdir -p contract/tests/build
ee pushd contract/tests
ee pushd build
ee "cmake -Deosio_DIR=$Deosio_DIR .."
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend adding -DCMAKE_BUILD_TYPE=Release. It shouldn't matter much, but every little bit helps.

Also, did you need the -Deosio_DIR thing? afaik that should not be required

Copy link
Member

Choose a reason for hiding this comment

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

Actually just tested, the difference is more on the order of 25%, so highly recommend doing Release here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, did you need the -Deosio_DIR thing? afaik that should not be required

Apparently not, though my previous testing had indicated otherwise. Removed.

Copy link
Contributor Author

@kj4ezj kj4ezj Feb 8, 2023

Choose a reason for hiding this comment

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

I would recommend adding -DCMAKE_BUILD_TYPE=Release. It shouldn't matter much, but every little bit helps.

Done. Thank you!

@heifner
Copy link
Member

heifner commented Feb 3, 2023

Not sure if it should be part of this PR, but alone with unittests would like to have tests/leap/nodeos_trust_evm_test.py run as part of CI/CD.

@kj4ezj
Copy link
Contributor Author

kj4ezj commented Feb 8, 2023

I have opened pull request 312 for test work.

@kj4ezj kj4ezj merged commit 1265423 into main Feb 9, 2023
@kj4ezj kj4ezj deleted the zach-contract-ci branch February 9, 2023 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD Continuous integration, continuous deployment - build and test system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants