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

Adding CI for building the waterbox and as many emulator cores as possible #3903

Merged
merged 48 commits into from
May 29, 2024

Conversation

SergioMartin86
Copy link
Contributor

@SergioMartin86 SergioMartin86 commented Apr 27, 2024

This a proposal to add Github Actions to build all waterboxed (and perhaps also the ported cores) to ensure they still compile correctly.

Check if completed:

  • I have run any relevant test suites

  • I, the committer, have read the licensing terms for contributors (last updated 2024-03-20) and am compliant

  • (perhaps for another PR) add the directly ported cores as well (e.g., quickerNES)

@Morilli
Copy link
Collaborator

Morilli commented Apr 28, 2024

This looks generally good; I do think we want to have consistent, reproducable builds for waterbox cores especially.

A couple immediate thoughts:

  • we probably want a way to not build mame. Not sure if it's worth to have a separate job for each individual core, but at the very least there should be a distinction of mame / not mame
  • you're setting the env variable a lot, it should ideally only be required to set CC for the musl build step; everything beyond should still work correctly
  • I wasn't sure whether we'd want to setup some kind of caching for the musl/libcxx build before, but looking at the build times from those jobs, it seems to complete in <2 minutes anyways? So I guess that won't be necessary.
  • I noticed this in the libcxx build logs:
Run ./do-everything.sh
Submodule 'waterbox/llvm-project' (https://github.com/llvm/llvm-project.git) unregistered for path '../llvm-project'
Submodule 'waterbox/llvm-project' (https://github.com/llvm/llvm-project.git) registered for path '../llvm-project'
  • ^ this sounds like it's re-cloning the llvm-project submodule using the setup-llvm.sh, which it shouldn't need to do... might investigate myself later
  • Would be nice to a have build artifacts containing the built waterbox cores. I could envision a future where we have core binaries built by CI and be retrievable by some other way than being rebuilt and committed to the main repo every time.

@YoshiRulz
Copy link
Member

@SergioMartin86 SergioMartin86 marked this pull request as draft April 28, 2024 08:54
@SergioMartin86 SergioMartin86 marked this pull request as ready for review April 28, 2024 12:27
@SergioMartin86
Copy link
Contributor Author

This looks generally good; I do think we want to have consistent, reproducable builds for waterbox cores especially.

A couple immediate thoughts:

  • we probably want a way to not build mame. Not sure if it's worth to have a separate job for each individual core, but at the very least there should be a distinction of mame / not mame
  • you're setting the env variable a lot, it should ideally only be required to set CC for the musl build step; everything beyond should still work correctly
  • I wasn't sure whether we'd want to setup some kind of caching for the musl/libcxx build before, but looking at the build times from those jobs, it seems to complete in <2 minutes anyways? So I guess that won't be necessary.
  • I noticed this in the libcxx build logs:
Run ./do-everything.sh
Submodule 'waterbox/llvm-project' (https://github.com/llvm/llvm-project.git) unregistered for path '../llvm-project'
Submodule 'waterbox/llvm-project' (https://github.com/llvm/llvm-project.git) registered for path '../llvm-project'
  • ^ this sounds like it's re-cloning the llvm-project submodule using the setup-llvm.sh, which it shouldn't need to do... might investigate myself later
  • Would be nice to a have build artifacts containing the built waterbox cores. I could envision a future where we have core binaries built by CI and be retrievable by some other way than being rebuilt and committed to the main repo every time.

Thanks for the suggestions. I think I addressed most, if not all, of these points.

@SergioMartin86 SergioMartin86 changed the title Adding CI for building all waterbox cores Adding CI for building the waterbox and as many emulator cores as possible Apr 28, 2024
Copy link
Collaborator

@Morilli Morilli left a comment

Choose a reason for hiding this comment

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

I do want to get this in master, if just for testing purposes. Couple nitpicks as this doesn't actually run correctly right now, but aside from that this looks good enough for now.

.github/workflows/make.yml Outdated Show resolved Hide resolved
.github/workflows/make.yml Outdated Show resolved Hide resolved
@Morilli
Copy link
Collaborator

Morilli commented May 28, 2024

In the current state, this workflow should now automatically run on every PR or push that modifies any waterbox/* file, and can alternatively be run on demand. Artifacts for the waterbox build itself, all waterbox cores and mame+quicknes will also be uploaded.

I think this is fine and can be merged as is, but I'm open to constructive feedback on whether having mame build that often is fine or not. If nothing comes up I'll probably just merge this later and see how it goes.

@vadosnaprimer
Copy link
Contributor

Are there any caps to potentially hit when building too much? MAME is theoretically too big (thankfully not as giant as non-arcade MAME), but if the wbx workflow changes it makes sense to rebuild them all too.

@SergioMartin86
Copy link
Contributor Author

SergioMartin86 commented May 28, 2024

Are there any caps to potentially hit when building too much? MAME is theoretically too big (thankfully not as giant as non-arcade MAME), but if the wbx workflow changes it makes sense to rebuild them all too.

I truly don't see a downside to building MAME every single time. GitHub CI is free, fast and parallel.

Ps. As far as I know there are no such caps. If there are, they must be super high to avoid abuse.

@Morilli Morilli merged commit 82c984b into TASEmulators:master May 29, 2024
1 check passed
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.

4 participants