Skip to content

[Build] Linux x86 runner#3

Merged
hughperkins merged 21 commits intomainfrom
hp/linux-x86-runner
Jun 4, 2025
Merged

[Build] Linux x86 runner#3
hughperkins merged 21 commits intomainfrom
hp/linux-x86-runner

Conversation

@hughperkins
Copy link
Collaborator

Issue: #

Brief Summary

Add linux x86 runner

  • runs on github standard runner
  • runs on any branch
  • for now it is 'run on demand', though I could update it to run on every commit, to a PR, eg could update that in this PR itself if we want

Notes:

  • Currently this doesnt include changes for tests to run cleanly (though I realized just now that the opengl changes are needed only for being able to run tests withou seg faulting I think, so we could kick that change ouf of this PR if we want?)
  • I found I needed to update changelog.py, in order to not crash when not making a release.

copilot:summary

Walkthrough

copilot:walkthrough

@hughperkins
Copy link
Collaborator Author

Note: we have to upgrade vulkan version, because old version no longer avialble.

@hughperkins hughperkins force-pushed the hp/linux-x86-runner branch from 13820c5 to ee692a5 Compare June 2, 2025 18:42
- uses: actions/checkout@v4
- name: Linux x86 Build
run: |
bash .github/workflows/scripts_new/linux_x86.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I do get the point of putting the CI script in a dedicated file instead of just pasting itself contain here. If the script is only used for CI, avoid unnecessary layers for indirection that are just obfuscating the logic.

Choose a reason for hiding this comment

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

It would also allow splitting it into multiple steps or jobs for better modularity and readability of failures.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so my understanding of the desires here are:

I in my turn personally like to factorize code out of the runner configuration because:

  • decouples code from runner configuration
  • means I can run/test the code easily, on arbitrary platforms, without needing to somehow trigger full CI flow etc
    • eg spin up a container, try running the script
    • run on my mac
    • etc ...
  • personally, I also find it easier to format code in standalone bash scripts, without needing to indent, pay attention to various yaml quirks etc
  • lastly, I feel that we shouldn't require potential developers to be able to read github runner config files, when setting up their development environment
    • yet I feel that the scripts used by CI are the best 'golden source' for what works today, when someone wants to install

To comply with all the above 😓 , what I have done is:

  • split the runner configuration into 4 steps:
    • prerequisites
    • build
    • install
    • test
  • split the script corresondingly into 4 parts, one part for each step
  • added documentation to the dev install page, linking to each of these steps, as a reference

Now I admit that we should ideally turn on the markup link checker, to run across all .md files, systematically, even if they didn't change, since someone could rename one of these scripts, and break the build page 😓 Well, baby steps. We do at least have the link checker running, and we'll turn it on, to run across all .md files, systematically, in a bit.

outdir.mkdir(parents=True, exist_ok=True)
unzip(local_cached, outdir, strip=strip)
elif name.endswith(".tar.gz") or name.endswith(".tgz"):
elif any(name.endswith(ext) for ext in [".tar.gz", ".tgz", ".xz", ".tar.bz2"]):
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change here, but I recommend immutable tuple instead of mutable list if mutability is irrelevant. better use the minimal required container, this makes the code more self-explanatory and may avoid silly mistakes (i.e. default values for method in Python).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Personally, I feel that list is more intuitive to read. tuple on the whole I feel represents something more analogous to a struct, even though it's technically iterable. I don't have a strong preference to keep it as a list, but I do have a gentle preference.

Default values for a method, totally agree with using tuple, instead of list, for the reason you allude to: that parameters to methods should not be mutaable, otherwise if you modify them inside the method, then you just modified thd efault parametrsr 😓

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess stachexchange tends to align with your own pov on this: when not mutable, use a tuple https://stackoverflow.com/questions/1708510/list-vs-tuple-when-to-use-each. i'll change to tuple

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, I might ask in #code

- uses: actions/checkout@v4
- name: Linux x86 Build
run: |
bash .github/workflows/scripts_new/linux_x86.sh

Choose a reason for hiding this comment

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

It would also allow splitting it into multiple steps or jobs for better modularity and readability of failures.

@hughperkins hughperkins force-pushed the hp/linux-x86-runner branch from 0a047ed to 451f379 Compare June 4, 2025 19:00
@hughperkins
Copy link
Collaborator Author

Thank you!

@hughperkins hughperkins merged commit db5ce62 into main Jun 4, 2025
23 of 26 checks passed
@hughperkins hughperkins deleted the hp/linux-x86-runner branch June 4, 2025 20:01
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.

3 participants