Skip to content

Conversation

@Sakimotor
Copy link

First time I do this so I don't know if my workflow is any good, and I did unashamedly cross-reference some github template examples with AI coding tools but after testing them the artifacts seem to run like intended.

@yell0wsuit
Copy link

Neat, but you might want to add the GitHub release step so it's shown in the Releases section.

@Sakimotor
Copy link
Author

Neat, but you might want to add the GitHub release step so it's shown in the Releases section.

Hmmmm I wonder if I've got it right, the if check might be futile but I don't see what other name I could give to the releases

@pnill
Copy link

pnill commented Mar 5, 2025

For future reference, instead of submitting a PR with 19 commits with the same title, you can always git rebase and squash them into a single commit. It looks nicer and easier to follow if the changes are simple/in a single file.

E.X. New commit making changes to same file on top of another commit
git rebase -i HEAD~2 (2 being the number of commits you're rebasing on from current HEAD) will open text editor and you can put a s to squash the new one into the old, then just git push -f to force it over the old history on github.

@halotroop2288
Copy link

You can also just squash-merge the PR to clean up the history while merging it.

@Sakimotor
Copy link
Author

Sakimotor commented Mar 6, 2025

For future reference, instead of submitting a PR with 19 commits with the same title, you can always git rebase and squash them into a single commit. It looks nicer and easier to follow if the changes are simple/in a single file.

E.X. New commit making changes to same file on top of another commit git rebase -i HEAD~2 (2 being the number of commits you're rebasing on from current HEAD) will open text editor and you can put a s to squash the new one into the old, then just git push -f to force it over the old history on github.

I will look into that, thanks. I usually give a minimal description to my commits, but this time I used the website's API so it ends up looking pretty messy and not very informative

Copy link
Member

@blueskythlikesclouds blueskythlikesclouds left a comment

Choose a reason for hiding this comment

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

This is great, thank you! Could you also add Release and RelWithDebInfo configurations to the matrix? I also don't really want GitHub releases to be created, so if you could remove that step it'd be great.

@halotroop2288
Copy link

I also don't really want GitHub releases to be created

Then where are you going to upload nightlies? Because GitHub Actions storing artifacts for a short period of time and requiring a sign-in to download is kinda cringe.

@blueskythlikesclouds
Copy link
Member

The recompiler is incomplete and will most likely require the user to make modifications to it. I don't see much point to providing prebuilt binaries.

- name: Install Ninja (Linux)
if: runner.os == 'Linux'
run: sudo apt-get update && sudo apt-get install -y ninja-build
shell: bash

Choose a reason for hiding this comment

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

Bash is the default shell for Linux.

Suggested change
shell: bash

- name: Install Ninja (Windows)
if: runner.os == 'Windows'
run: choco install ninja
shell: powershell

Choose a reason for hiding this comment

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

PowerShell is the default for Windows.

Suggested change
shell: powershell

Comment on lines +39 to +59
- name: Configure CMake (Linux)
if: runner.os == 'Linux'
run: >
cmake
-DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
-DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
-G Ninja
-S .
shell: bash

- name: Configure CMake (Windows)
if: runner.os == 'Windows'
run: >
cmake
-DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
-DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
-G Ninja
-S .
shell: powershell

Choose a reason for hiding this comment

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

These are the same.

Suggested change
- name: Configure CMake (Linux)
if: runner.os == 'Linux'
run: >
cmake
-DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
-DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
-G Ninja
-S .
shell: bash
- name: Configure CMake (Windows)
if: runner.os == 'Windows'
run: >
cmake
-DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }}
-DCMAKE_C_COMPILER=${{ matrix.c_compiler }}
-DCMAKE_BUILD_TYPE=${{ matrix.build_type }}
-G Ninja
-S .
shell: powershell
- name: Configure CMake
run: cmake -DCMAKE_CXX_COMPILER=${{ matrix.cpp_compiler }} -DCMAKE_C_COMPILER=${{ matrix.c_compiler }} -DCMAKE_BUILD_TYPE=${{ matrix.build_type }} -GNinja -S .

Comment on lines +72 to +74
cp XenonRecomp/XenonRecomp artifacts/
cp XenonAnalyse/XenonAnalyse artifacts/
shell: bash

Choose a reason for hiding this comment

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

Suggested change
cp XenonRecomp/XenonRecomp artifacts/
cp XenonAnalyse/XenonAnalyse artifacts/
shell: bash
cp XenonRecomp/XenonRecomp XenonAnalyse/XenonAnalyse artifacts

Comment on lines +116 to +117
draft: false
prerelease: false

Choose a reason for hiding this comment

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

These default to false.

Suggested change
draft: false
prerelease: false

Comment on lines +80 to +82
Copy-Item -Path XenonRecomp\XenonRecomp.exe -Destination artifacts\
Copy-Item -Path XenonAnalyse\XenonAnalyse.exe -Destination artifacts\
shell: powershell

Choose a reason for hiding this comment

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

Suggested change
Copy-Item -Path XenonRecomp\XenonRecomp.exe -Destination artifacts\
Copy-Item -Path XenonAnalyse\XenonAnalyse.exe -Destination artifacts\
shell: powershell
Copy-Item XenonRecomp\XenonRecomp.exe XenonAnalyse\XenonAnalyse.exe artifacts

uses: actions/upload-artifact@v4
with:
name: XenonBinaries-${{ matrix.os }}
path: artifacts/*

Choose a reason for hiding this comment

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

Suggested change
path: artifacts/*
path: artifacts/*
if-no-files-found: error

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.

6 participants