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

Move book examples into standalone projects. Include them in book via mdbook preprocessor. #766

Merged
merged 10 commits into from
Feb 15, 2022

Conversation

mitchmindtree
Copy link
Contributor

@mitchmindtree mitchmindtree commented Feb 10, 2022

Closes #544, follows the example set in #731.

This also adds all examples to the Sway workspace so that we can test both projects under CI. Doing so means the Cargo.lock for the workspace requires an update, but I haven't included it yet to reduce noise in the PR. @adlerjohn shall I add the Cargo.lock changes as a final commit in this PR? Or do you generally update the lock file separately?

TODO

  • hello_world
  • fizzbuzz
  • wallet_smart_contract
  • Ensure tests are run for examples in CI.
  • Ensure forc build is ran for each example in CI.

@@ -39,7 +31,7 @@ Here are the contents of the only Sway file in the project, and the main entry p
script;

fn main() {

Copy link
Contributor Author

@mitchmindtree mitchmindtree Feb 10, 2022

Choose a reason for hiding this comment

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

Woops, my editor's setup to remove trailing whitespace automatically when writing the buffer


// Generate Rust bindings from our contract JSON ABI
// FIXME: Incorrect path, see https://github.com/FuelLabs/fuels-rs/issues/94
abigen!(MyContract, "./examples/hello_world/my-contract-abi.json");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might need to resolve FuelLabs/fuels-rs#94 before landing this, as this path might be confusing for newcomers reading through the book.

Related to FuelLabs#544.
Follows the example set in FuelLabs#731.
Tested with `mdbook serve`.

Also adds the `hello_world` and `subcurrency` examples to the Sway
workspace so that we can test both projects under CI.
@adlerjohn
Copy link
Contributor

If you're going to update the workspace or Manifest file deps, then you can update the lockfile in the same PR. That being said, I'm actually against putting the examples in the workspace because that would modify the lockfile. We had to remove examples from fuel-core and move them to a whole new repo to avoid dependency issues. My preference would be instead to add a new task to the CI pipeline to go into each example sub-directory and test it.

@adlerjohn adlerjohn added ci The Sway Book Everything to do with the Sway Book labels Feb 10, 2022
@mitchmindtree
Copy link
Contributor Author

We had to FuelLabs/fuel-core#140 and move them to a whole new repo to avoid dependency issues.

Aghhhhh I see, that makes sense. Things get tricky when you build the thing you use to build things in the same repo 😂

My preference would be instead to add a new task to the CI pipeline to go into each example sub-directory and test it.

Sounds good - I'm in the middle of writing a little rust "script" that runs forc build for each forc project under the examples directory solely for the purpose of running in CI. I'll try modifying it to also run the tests manually for each so we can remove them from the workspace altogether.

examples/build-all-examples/Cargo.toml Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
This script walks the examples directory and, for every directory
containing a `forc.toml` file, runs `forc build` for that directory.
Examples that encounter a failure of some kind have their `stdout` and
`stderr` piped through so we can easily identify cause of failure in CI.

This also removes the examples from the workspace.

Currently unsure whether or not the added CI pass to install `forc` will
result in `forc` actually being available on the PATH in the CI worker,
but will find out soon.

Still need to work out how to run `forc test` for each project without
having cargo complain about building within the parent sway workspace.
Currently, it's tricky to get an idea of how many successes/failures
there were without combing through the stdout/stderr. This adds a little
summary to the end that looks something like this:

```
Build all examples summary:
  [✓]: /home/mindtree/programming/rust/fuel/sway/examples/fizzbuzz succeeded!
  [✓]: /home/mindtree/programming/rust/fuel/sway/examples/hello_world succeeded!
  [x]: /home/mindtree/programming/rust/fuel/sway/examples/wallet_smart_contract failed!
  [✓]: /home/mindtree/programming/rust/fuel/sway/examples/subcurrency succeeded!
3 successes, 1 failure
```
@mitchmindtree mitchmindtree marked this pull request as ready for review February 11, 2022 01:18
@mitchmindtree
Copy link
Contributor Author

OK, I think this is just about ready to go!

Testing examples

I think it's best to leave the example testing (i.e. running forc test for each) to a future PR as there are a few issues involved that should be worked through first. I've opened #771 to track this.

wallet_smart_contract failure

Currently the wallet_smart_contract example fails during forc build as storage isn't yet implemented. We could leave this PR up until storage lands (it sounds like its not far off?) or add a hack to ignore this example for now?

@adlerjohn
Copy link
Contributor

We could leave this PR up until storage lands (it sounds like its not far off?) or add a hack to ignore this example for now?

I'd say just comment out the offending lines and add a comment to the effect of "storage not yet implement, see this here PR, and here's how it would look if it were implemented." That way all examples at least compile.

Currently it takes CI another 7 mins to build and install forc from
scratch. This should allow `cargo` to re-use the artifacts it has
already built during the sway building and testing steps earlier in the
CI job.
@mitchmindtree
Copy link
Contributor Author

Okydoke, I had to comment out a large portion of the wallet example, but I've left a NOTE at the top with a link to the PR. Once this lands I'll open a dedicated issue for updating the wallet example.

Copy link
Contributor

@sezna sezna left a comment

Choose a reason for hiding this comment

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

Generally LGTM, left some nits

examples/fizzbuzz/Forc.toml Outdated Show resolved Hide resolved
examples/hello_world/Cargo.toml Outdated Show resolved Hide resolved
examples/hello_world/Forc.toml Outdated Show resolved Hide resolved
examples/wallet_smart_contract/Cargo.toml Outdated Show resolved Hide resolved
examples/wallet_smart_contract/Forc.toml Outdated Show resolved Hide resolved
@mitchmindtree
Copy link
Contributor Author

@sezna good catch, should be addressed now!

@adlerjohn adlerjohn requested a review from sezna February 13, 2022 23:14
@mitchmindtree mitchmindtree merged commit e10acd6 into FuelLabs:master Feb 15, 2022
@mitchmindtree mitchmindtree deleted the book-example-extraction branch February 15, 2022 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci The Sway Book Everything to do with the Sway Book
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Move docs code examples to real projects
3 participants