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

cargo check modules, runtimes, and node. #72

Closed
wants to merge 25 commits into from
Closed

Conversation

JoshOrndorff
Copy link
Owner

This is a draft of getting CI to check each module. My approach so far is adding a line in the .circleci/config.yml for each module. This has a significant disadvantage that the builds don't share a common target directory so the common stuff gets recompiled every time. I think the solution involves workspaces. Check out how the main substrate repo organizes all those modules into the srml directory. But I'm not sure how to do that.

I'd also like to get the cacheing working so the CI doesn't have to download so much stuff each time. @jimmychu0807 I know you took care of the caching on the front end template. Any advice to share?

@JoshOrndorff
Copy link
Owner Author

Ooh, maybe we need a virtual manifest https://stackoverflow.com/a/49463321/4184410

@shawntabrizi
Copy link
Contributor

It is possible to use each module in a trivial way from a single file?

For example, import the module, and do something with the Module struct. Might be enough to cause it all the modules to compile and run tests and stuff from a single source.

@JoshOrndorff JoshOrndorff changed the title Joshy ci build cargo check modules, runtimes, and node. Oct 26, 2019
@JoshOrndorff
Copy link
Owner Author

Got it working much more nicely now with a virtual workspace.

@JoshOrndorff
Copy link
Owner Author

Hmmm, latest commit breaks CI but passes locally. The error log in ci sounds like maybe our build got killed by a process manager? I wonder if we have limited resources or something? I'm gonna rest on this one for a little while.

@JoshOrndorff
Copy link
Owner Author

I decided to switch this back to just checking the modules so that at least something was checked in the short term and enlisting the help of Parity's devops team. But now even the module CI fails.

In particular it fails with

error[E0061]: this function takes 4 parameters but 3 parameters were supplied
  --> modules/reservable-currency/src/lib.rs:66:13
   |
66 |             T::Currency::transfer(&sender, &dest, amount)?;
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 4 parameters

which doesn't make any sense because:

@gabreal
Copy link

gabreal commented Nov 15, 2019

I can reproduce this error locally with rust version:

stable-x86_64-unknown-linux-gnu unchanged - rustc 1.39.0 (4560ea788 2019-11-04)
nightly-x86_64-unknown-linux-gnu updated - rustc 1.41.0-nightly (82cf3a448 2019-11-14)

@JoshOrndorff
Copy link
Owner Author

Indeed this is a legitimate fail. I now see the signature of transfer from the upstream Substrate that we've pinned does indeed expect 4 parameters https://github.com/paritytech/substrate/blob/master/paint/support/src/traits.rs#L399

Why this previously passed locally is a mystery, and why the rustdocs are out of date is also a mystery. This particular failure is addressed in #88

Although I'm still having trouble building the node end-to-end. Once the build is working again locally, I'll return to pursuing the CI.

@JoshOrndorff
Copy link
Owner Author

JoshOrndorff commented Nov 20, 2019

Okay, so it seems checking the entire node is just too much for CircleCI. I'll remove the node check.

Results from https://circleci.com/gh/substrate-developer-hub/recipes/112?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

note: : final link failed: Cannot allocate memory

@JoshOrndorff
Copy link
Owner Author

@4meta5 There's a green checkmark! Long time coming. If this looks good to you, let's merge it. Then I can follow up with @gabreal about a better and more maintainable CI pipeline.

@JoshOrndorff
Copy link
Owner Author

These commits all got merged with #90, but github didn't figure that out, so I'll just close this one.

@JoshOrndorff JoshOrndorff deleted the joshy-ci-build branch February 3, 2020 19:07
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

3 participants