Skip to content

Minimize CI burden by commenting out parts of the matrix #93

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paddy-exe
Copy link
Collaborator

@paddy-exe paddy-exe commented Jun 17, 2025

  • Minimizes CI burden by commenting out parts of the matrix so that one entry for every platform remains
Before-After comparison

Before:

image

After:

image

  • Adds a commented out option for web builds using threads=no

@paddy-exe paddy-exe requested a review from a team June 17, 2025 22:44
@paddy-exe paddy-exe added the enhancement New feature or request label Jun 17, 2025
@paddy-exe paddy-exe changed the title Minimize CI burden by commenting out parts of the matriix Minimize CI burden by commenting out parts of the matrix Jun 17, 2025
@Ivorforce
Copy link
Member

Ivorforce commented Jun 17, 2025

I'm not sure I like disabling CI, it means users will have to re-enable these builds manually with a commit when cloning the repo.
Since most time spent is compiling godot-cpp, a more effective way to speed up CI would be to use a build profile. We could configure the profile from the repository variables, such that it is only in effect for the template repo, but not for cloned projects. What do you think?

Edit: Actually, I'd disable double builds by default (aka half of CI). I don't think many people are using that anyway.

@paddy-exe
Copy link
Collaborator Author

paddy-exe commented Jun 17, 2025

I'm not sure I like disabling CI, it means users will have to re-enable these builds manually with a commit when cloning the repo. Since most time spent is compiling godot-cpp, a more effective way to speed up CI would be to use a build profile. We could configure the profile from the repository variables, such that it is only in effect for the template repo, but not for cloned projects. What do you think?

Hmm yeah I guess you could be right. On the other hand probably not everyone would be wanting to have everything enabled by default anyways. Not every extension will (need to) work on every platform. Also one commit to enable everything doesn't sound very bad in my opinion. You will probably want to configure your stuff anyways since we have the naming predefined with the template variables. So might as well... I am not opposed to build profiles though. Haven't heard about them though (I only get weird search results). Do you mind sharing a link to a docs page?

Edit: Actually, I'd disable double builds by default (aka half of CI). I don't think many people are using that anyway.

Even better. So that would change the line

float-precision: [single, double]

to this here then?:

float-precision: [
single, 
#double
]

@Ivorforce
Copy link
Member

Ivorforce commented Jun 17, 2025

I am not opposed to build profiles though. Haven't heard about them though (I only get weird search results). Do you mind sharing a link to a docs page?

I don't think they're documented yet :/

They're used to compile only parts of godot-cpp, and skip the rest. You can use one with the build_profile=some/path/to/build_profile.json command.
They look like this: https://github.com/Ivorforce/NumDot/blob/main/configure/build_profile.json

I think they need to be in a file, but as long as users don't manually use the build_profile command, they won't 'have to deal' with most classes mysteriously missing when they first test it out.

Edit: I just remembered, we do actually have a dummy build_profile.json in this repository.

Even better. So that would change the line

float-precision: [single, double]

to this here then?:

float-precision: [
single, 
#double
]

Yep, that should work!

@Bromeon
Copy link

Bromeon commented Jun 18, 2025

You can use include: to add individual matrix combinations, without running into combinatorial explosion. This would allow double builds to still be tested, just not for all other dimensions.

We follow a similar approach in godot-rust. We run 3 double jobs in total, one per Desktop platform.

- Minimizes CI burden by commenting out parts of the matrix so that one
entry for every platform remains
- Adds a commented out option for web builds using threads=no
@paddy-exe paddy-exe force-pushed the minimize-ci-burden branch from b9003b0 to 122e8c6 Compare June 19, 2025 14:41
Comment on lines +44 to +52
include:
- platform: linux
arch: x86_64
os: ubuntu-22.04
float-precision: double
- platform: web
arch: wasm32
os: ubuntu-22.04
threads: no
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Bromeon You mean like this? I took a look at gdext and the docs page you linked

Copy link

Choose a reason for hiding this comment

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

Yes, does it work as expected?

Small side note, the [ ] syntax across multiple lines is a bit unusual in YAML, although allowed. Typically lists are - prefixed like here 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really sure :/ the action on GitHub doesn't run (and doesn't show any debug info either). I tried to run the action locally but I can't run everything there with Docker either 😅

Copy link

Choose a reason for hiding this comment

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

It's here, in the Actions tab of the repo 🙂

Build GDExtension
Error when evaluating 'runs-on' for job 'build'. .github/workflows/builds.yml (Line: 54, Col: 14): Unexpected value ''

The problem is that target is nested (itself a list of objects containing platform/arch/os), but you only refer to its sub-keys.

The include however needs to refer to paths from the top level, like target-type. I'm not sure how the exact syntax would work for a nested property, maybe

    include:
      - target:
            platform: linux
            arch: x86_64
            os: ubuntu-22.04
        float-precision: double

GitHub Actions are always trial&error, you might need to experiment a bit (or ask an AI, which itself can be trial&error) 😬 But it's also possible that this isn't supported, and would need flat keys.

Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Personally, I use the GH runners to make builds, and I trigger CI manually only once every month or so. So I'd definitely prefer CI that has all the templates enabled by default, and with a nice and clean configuration — but also have it not auto-run on push.
But in this repository, it's set to run by default. I suppose with that configuration, it makes sense to only check a few combinations.

So yea, even though I personally don't like the change much, i guess it's a net plus in most situations.
We should probably still be making that build profiles change though, at least here. That should save us another ~80% or so of build time. But I'm happy to do that as follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants