Build and test in a matrix

[wiktor-k]

No description provided.

[Jakuje]

To catch the #282, we would need to also run the tests on this target.

[wiktor-k]

To catch the #282, we would need to also run the tests on this target.

In this case, to be precise we don't actually need to run tests we just need to compile them. cargo check ... --all-targets checks tests and examples too (see cargo targets) and IMO we were missing that.

I think we don't need to strictly cargo build since we throw away the artifacts so check is sufficient. Running the tests may also be a good idea (but separate from the check) although I'm not sure how does it work (I don't think it emulates, say, aarch64 to run the test).

I've updated the matrix job and it's clearly visible it's not just that particular 32bit target that was failing.

To be honest I'd move the ci.sh to yaml since it's pretty straightforward and GitHub shows separate sections for all steps but I don't know how much is @hug-dev attached to that 😉

[wiktor-k]

I've cherry-picked your commits from the other PR... let's see how many tests will be green now :)

[Jakuje]

Sounds like all of them were fixed :)

I like the idea of running on the whole matrix. It makes it easier to catch all possible oddness (but really the most common one is the ulong size which demonstrates on 32b arches). The linux-x86_64 is missing though.

On the other hand, I would probably be interested in running the test on 32b arch too (can be separate PR based on what I did in #283). We can probably do it easily only on i386 though. We might get the arm64 runner as mentioned in #285 and we might be able to run there the arm32 binaries, but I never played around this and I an skeptical it will show other errors than the i386 ones.

[wiktor-k]

Okay, I've simplified it further: noticed that we're checking formatting twice (once in a job, second time in ci.sh) and removed the one in ci.sh. I've also extracted clippy into separate job. The same for unit tests which (for now?) use a very limited target list (which should mimic the old behavior).

See if this looks any better and thanks for review! 👋

Btw, we won't be able to merge this anyway until @hug-dev changes the definition of "Required" checks (they're hardcoded for now to look for old jobs).

[Jakuje]

looks good!

[hug-dev]

yooohooo amazing job! Looks much better than our ci.sh script! Definitely in favour of removing it if we can do everything from the yaml file :) Maintaining a big shell file is not fun...

[hug-dev]

Modified the branch protection rules to remove the old check. I however need to manually add all matrix variations in the branch protection rules (that is 38 checks). There are some way around but they do not look too good.

Shall I manually add all of them as required or do we only require a subset?

[wiktor-k]

Phew, great to hear that @hug-dev, I was worried how would you react 😉 but I think this should vastly improve our testing coverage :)

I've dropped the two commits from @Jakuje as they seem to be addressed by the Windows PR merged recently.

Thanks for help, deeply appreciated 🙏

[wiktor-k]

I however need to manually add all matrix variations in the branch protection rules (that is 38 checks). There are some way around but they do not look too good.

Yes, this is super annoying and I didn't find a satisfactory workaround (also in my other repos that have dozens of checks), gee GitHub, can't we just say "select them all"? :-/

Shall I manually add all of them as required or do we only require a subset?

I think a subset would be fine (and no worse than what we had previously) but of course ideally we would want to have them all... well, I always wait for the full CI before hitting approve anyway 😅

[Jakuje]

Yes, this is super annoying and I didn't find a satisfactory workaround (also in my other repos that have dozens of checks), gee GitHub, can't we just say "select them all"? :-/

We are using the "dummy" job , that will depend on the whole matrix (can be referenced by just the name in the yaml and that can be "required" in the gui to pass in other project and it does not look that bad:

https://github.com/ansible/pylibssh/blob/devel/.github/workflows/ci-cd.yml#L1427

[wiktor-k]

We are using the "dummy" job , that will depend on the whole matrix (can be referenced by just the name in the yaml and that can be "required" in the gui to pass in other project and it does not look that bad:

re-actors/alls-green@release/v1 looks great! I've added that to CI and I think now it can be selected as being "required". Thanks!

[hug-dev]

Nice! Added the new check in the required workflows :)