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

Shader compilation in GitHub Actions #178

Merged
merged 4 commits into from
Jul 13, 2022
Merged

Shader compilation in GitHub Actions #178

merged 4 commits into from
Jul 13, 2022

Conversation

raphlinus
Copy link
Contributor

This patch sets up very basic CI (right now just cargo fmt) but more importantly compiles shaders in a GitHub Action.

Any PR to branches other than main will run shader compilation. Any push to the dev branch will run shader compilation and then merge to main.

Closes #177

This patch sets up very basic CI (right now just cargo fmt) but more importantly compiles shaders in a GitHub Action.

Any PR to branches other than main will run shader compilation. Any push to the dev branch will run shader compilation and then merge to main.

Closes #177
@raphlinus
Copy link
Contributor Author

The CI failed for a technical reason (it doesn't run shader compilation on main but the PR is against master; I want to do the rename but haven't yet). Possibly I could do more fine-tuning to make this more robust, or in any case produce better error messages.

name: compile shaders
steps:
- uses: actions/checkout@v3
- run: mkdir piet-gpu/shader/gen
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding -force here should suppress the error if the directory already exists. It might be worth not doing this though and just rejecting any PRs that contain the gen directory?

@dfrg
Copy link
Collaborator

dfrg commented Jul 10, 2022

I'm not the most qualified to review GitHub actions, but nothing here stands out as problematic to me.

I assume that after this lands, all PRs should be directed to dev?

@raphlinus
Copy link
Contributor Author

Yes. I think something that should happen in this PR is that the README gets updated with clear language describing what's going on. People will want main if they're building, but dev if they're either making updates or wanting to look at clean PRs without the noise of generated compiled shaders. There's obviously a ton more that can be done in CI as well, but my current thinking is to land this with minimal changes (aside from the README) and add more as followup PRs.

One other question is that we have a few more gen directories (tests, examples for piet-gpu-hal). Should we do all of these so it's consistent?

Also remove 'shader/gen' gitignore line in main (it's present in dev).
Explain the shader compilation approach. Also add links while I'm at it.
Copy link
Collaborator

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

This all looks good to me. Now seems as good a time as any to make the change.

@raphlinus raphlinus merged commit b77df99 into main Jul 13, 2022
@raphlinus raphlinus deleted the ci2 branch July 13, 2022 19:21
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.

Shader compilation plan
2 participants