Setup Kinobi (2/2)

[buffalojoec]

This is PR 2/2 for setting up Kinobi and the rest of the goodies from the
create-solana-program template.

First I laid down the boilerplate that comes from create-solana-program,
then added shank to the program to generate an IDL, then generated a
client.

After that, I split formatting and linting into separate scripts. In Rust, that's
rustfmt and clippy, and in JavaScript, that's prettier and eslint,
respectively.

Finally I bootstrapped a CI pipeline according to @febo's new workflow design:

[lorisleiva]

I think splitting formatting and linting is okay in the scripts but is it necessary in CI? I'm just trying to see if that's what we'd want to do in the create-solana-program. On one side we make the CI pipeline slightly more overwhelming, on the other we may save some pipeline minutes by failing slightly faster. What do we think?

[buffalojoec]

I think splitting formatting and linting is okay in the scripts but is it necessary in CI? I'm just trying to see if that's what we'd want to do in the create-solana-program. On one side we make the CI pipeline slightly more overwhelming, on the other we may save some pipeline minutes by failing slightly faster. What do we think?

It doesn't have to be split in CI, it can be "Format and Lint", doesn't really matter.

[buffalojoec]

I will say that, although it looks a bit neater to split them in CI, one compelling reason is the fact that, in Rust, formatting does not require building the crate, however clippy does. In other words the formatting job can be faster and doesn't need caching, while clippy will have a bit more overhead and utilize a cargo cache.

[lorisleiva]

Yes but if you have the formatting step occurs before the linting step on the same job, we have a very similar behaviour as, if the formatting step fails, the job will stop and other jobs in the same group will be cancelled. In a way, it's more performant because it's less sequential — i.e. we don't need to wait until all formatting steps are done before moving on to the linting phase.

[buffalojoec]

I think it's just a matter of preference, really. You're right it might be faster if they're grouped together. I typically lean towards "one job should do one specific thing", but maybe we don't want to impose that on the template.

Somebody break the tie @febo !

[lorisleiva]

I think we just have a slightly different view of the granularity of the "one thing" that each CI job does. For me, formatting and linting is part of the same "one thing" which is "check the code style is alright". But I can also see your point of view that formatting can be its own thing — i.e. "check the code style statically" — and linting its own other thing — i.e. "check the code style is alright in a more clever way which, in Rust, means actually building the code". I still leaning towards the former but selecting the latter is also fine by me.

[febo]

I think it's just a matter of preference, really. You're right it might be faster if they're grouped together. I typically lean towards "one job should do one specific thing", but maybe we don't want to impose that on the template.

Somebody break the tie @febo !

I am inclined to "merge" formatting and linting into the same step, where both are related to checking code style. It is also faster by reducing the sequential steps and less jobs being started, which trigger loading an image, cache, etc.

[febo]

I would also group the "Check IDL generation" and "Check client generation" together with "Build programs" since you probably don't want to test the client if any of them fail. As it is, the client testing will continue even when they fail.

[buffalojoec]

Alrighty, majority rules! I've added a commit to combine formatting and linting, as requested.

I would also group the "Check IDL generation" and "Check client generation" together with "Build programs" since you probably don't want to test the client if any of them fail.

Makes sense to me! Originally I was keying on the fact that you don't need to build the program to generate the IDL. However, I do think the two generation checks can be the same job, since if the IDL check fails, the client generation check should also fail, but should be skipped/aborted. I've added a commit for this as well.

How are we feeling about this pipeline and the yaml files as it pertains to the create-solana-program template? The goal here is to get a pattern going that we'll use for all of our programs and ideally the template, so lmk any adjustments.

[lorisleiva]

Nice! I think this is probably good enough for this specific program but if we're gonna move this into create-solana-program then I have some concerns.

I do think the two generation checks can be the same job.

Whilst I agree there is technically a flow of first generating the IDL and then generating the clients, the checks can be run in parallel here because we are failing if the working directory is dirty at each step. The issue with running them sequentially is that the "Check IDL" job is much slower as Anchor or Shank will end-up building the program (which is why in the create-solana-program CI it is run after the "Build programs" step so it can reuse the cache). Parallelising them in the same group should practically make the pipeline much faster because if "Check clients" fail (which is super fast), "Check IDL" will be cancelled before it can finish.

Additionally, I think the "Test programs" job has no need to wait for the "Build programs" job as they use two different sets of cache (one built for prod and one built for dev). Having them sequentially will significantly increase the duration of the pipeline.

---

To summarise, my recommendation is to:

• Keep the "Check IDL" and "Check client generation" jobs separate.
• Move the "Check IDL" and "Check client generation" jobs after the "Build programs" job so they can use its cache.
• Move the "Test programs" job so it is parallel to the "Build programs" job.

[buffalojoec]

the "Check IDL" job is much slower as Anchor or Shank will end-up building the program

I'm not sure about Anchor, but Shank does not build the program. It builds the Shank crate(s) and then reads the files in your crate. It uses the derive macro annotations to key on, but they only expand at compile-time to give you things like helpers.

To summarise, my recommendation is to:

• Keep the "Check IDL" and "Check client generation" jobs separate.

Yep I'm okay with this. I liked having them separate for the same reasons you listed.

• Move the "Check IDL" and "Check client generation" jobs after the "Build programs" job so they can use its cache.

For Shank they don't need the program's cached build, so do they still need to depend on "Build programs"? It seems like the 3rd job from my original pipeline is good.

• Move the "Test programs" job so it is parallel to the "Build programs" job.

Sounds good!

[lorisleiva]

I'm not sure about Anchor, but Shank does not build the program

Ah okay then it seems it'd only be an optimisation for Anchor programs. Therefore maybe it's good enough to leave it in parallel with the build program job. 👍

P.S.: Once we have Kinobi macros, we won't even need this "Check IDL generation" step since it'll be created on program build.

[buffalojoec]

Ok @lorisleiva I've updated with your feedback. The only question I have is, do the other tests need the program build step? I would assume so, right? So even though test_program might be able to run in parallel with build_program, the client tests probably can't?

[lorisleiva]

Yeah, clients need to be tested after the "Build programs" job because they start a local validator using the .so files created by the build job.

Technically only the JS client currently uses a local validator since the Rust client doesn't need to but since any other future client will end up needing a validator I think it's best to have them all together after the build job. Wdyt?

[febo]

Yeah, clients need to be tested after the "Build programs" job because they start a local validator using the .so files created by the build job.

Technically only the JS client currently uses a local validator since the Rust client doesn't need to but since any other future client will end up needing a validator I think it's best to have them all together after the build job. Wdyt?

The Rust client tests needs to run after "Build programs" as well since it uses the .so file from the program.

[buffalojoec]

Okay, agreed with both of you! I'll move them after. Let me know if there's any other feedback on this overall setup, as it pertains to create-solana-program.

[lorisleiva]

@buffalojoec I've got lots of smaller notes — e.g. is a matrix really necessary if we're going through each variant to select the script anyway — but these are nits that would only apply to porting these changes to create-solana-program. Maybe we could have a quick chat to iron out all the details in one go to prevent this PR from being delayed even further?

Otherwise I'm also happy to make a PR to create-solana-program that ports some of the changes of this PR and you can let me know what you think.

[buffalojoec]

@buffalojoec I've got lots of smaller notes — e.g. is a matrix really necessary if we're going through each variant to select the script anyway — but these are nits that would only apply to porting these changes to create-solana-program. Maybe we could have a quick chat to iron out all the details in one go to prevent this PR from being delayed even further?

Otherwise I'm also happy to make a PR to create-solana-program that ports some of the changes of this PR and you can let me know what you think.

Sounds good to me! No worries on blocking this PR, this whole intention of this PR itself was to bikeshed what goes into create-solana-program, then PR everything we wanted into the template, then apply the template to all of our program repos. So no rush, really.

Yeah a chat sounds good to me!

[lorisleiva]

Alright I've thrown lot's of rough notes I have about these changes as they are meant to be applied to create-solana-program. Let us discuss. 😄

[buffalojoec]

I think I've addressed everything! I added our discussed changes one commit at a time, then the last commit drops all customization and aims to demonstrate what the template might put out.

FYI the "Test Programs" job is going to fail until we stick the configs back in (ie. --features bpf-entrypoint). The output is truncated.

[2024-07-19T22:57:01.413223000Z DEBUG solana_runtime::message_processor::stable_log] Program is not deployed
[2024-07-19T22:57:01.413230000Z DEBUG solana_runtime::message_processor::stable_log] Program Feature111111111111111111111111111111111111 failed: invalid account data for instruction
test test_revoke_pending_activation ... FAILED

failures:

---- test_revoke_pending_activation stdout ----
thread 'test_revoke_pending_activation' panicked at program/tests/functional.rs:86:5:
assertion `left == right` failed
  left: InstructionError(0, InvalidAccountData)
 right: InstructionError(0, MissingRequiredSignature)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    test_revoke_pending_activation

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.06s

error: test failed, to rerun pass `--test functional`

[buffalojoec]

Merging on red so I can submit a PR to update the template, which should just... fix everything. 😁