-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
GitHub Actions CI Port #16082
base: master
Are you sure you want to change the base?
GitHub Actions CI Port #16082
Conversation
What's the motivation for porting to GitHub Actions? Doesn't it have the exact same restrictions as Azure? |
1cdde6d
to
1b3a7cd
Compare
so users don't have to sign up to extra services to run auto builds, and yeah i think |
I think that GitHub Actions is useful now because of free arm64 runners. I improved illusion0001's version a bit: |
|
I know that)
Actions x86_64 has this lines in log:
But Actions arm64 has only this lines in log:
|
Ok. I fixed arm64 build with simple |
Maybe instead of matrix: Linux Build it can be matrix: Linux Aarch 64 and matrix: Linux X64 by creating new name categories. |
Pushed |
Arm gcc build is clang, needs fixing |
I tested few changes and this one works pretty good: |
Updated. Also fixed windows artifact uploading build files |
f198291
to
11a5643
Compare
You can use nightly.link with it. |
.github/workflows/rpcs3.yml
Outdated
matrix: | ||
compiler: [clang, gcc] | ||
cfg: | ||
- os: ubuntu-24.04-arm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To save on CI time, we only want to run the clang compiler on arm, since we're already testing both compilers on the x86 build. For arm64 just use the compiler we want to publish binaries with (clang).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed
.github/workflows/rpcs3.yml
Outdated
- name: Append Compiler version to appimage | ||
run: | | ||
file=".ci/deploy-linux.sh" | ||
sed -i 's/RPCS3_APPIMAGE="rpcs3-v${COMM_TAG}-${COMM_COUNT}-${COMM_HASH}_linux64.AppImage"/RPCS3_APPIMAGE="rpcs3-v${COMM_TAG}-${COMM_COUNT}-${COMM_HASH}-${COMPILER}_linux64.AppImage"/g' $file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you changing the output filename here? If we want to change the output filename just change the deploy script. I'm not sure why you're changing it in the first place though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to separate the artifacts from being overwritten, i.e clang finishes before gcc, when GCC is uploaded, clang appimage is overwritten. I didn't want to modify script to keep compatibility for other ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, now that I had a second look at this, it's not needed after all because artifact have compiler name in it
${{ matrix.cfg.build_sh }} | ||
- name: Upload artifacts | ||
uses: actions/upload-artifact@main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with actions/upload-artifact@main
, is this going to upload binaries to RPCS3/rpcs3-binaries-win/linux/linux-arm64/etc ? I don't see where we set any variables related to the upload repository, so I don't think this will work how we want it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also only run this step with the same restrictions we use in Azure:
condition: and(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['Build.Repository.Name'], 'RPCS3/rpcs3'), eq(variables['Build.SourceBranch'], 'refs/heads/master'), eq(variables['COMPILER'], 'clang'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
if: |
github.event_name != 'pull_request' &&
github.repository == 'RPCS3/rpcs3' &&
github.ref == 'refs/heads/master' &&
matrix.compiler == 'clang'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with
actions/upload-artifact@main
, is this going to upload binaries to RPCS3/rpcs3-binaries-win/linux/linux-arm64/etc ? I don't see where we set any variables related to the upload repository, so I don't think this will work how we want it to.
Nope, there will have to be a separate step for it. Using appropriate tokens like azure to upload it
.github/workflows/rpcs3.yml
Outdated
DEPLOY_APPIMAGE: true | ||
APPDIR: "/rpcs3/build/appdir" | ||
ARTDIR: "/root/artifacts" | ||
RELEASE_MESSAGE: "/rpcs3/GitHubReleaseMessage-${{ matrix.compiler }}.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to have release messages per-compiler, since we only upload the binaries from one compiler to the GitHub Release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
uses: actions/cache@main | ||
with: | ||
path: ${{ env.CACHE_DIR }} | ||
key: ${{ runner.os }}-${{ env.COMPILER }}-${{ env.QT_VER }}-${{ env.VULKAN_SDK_SHA }}-llvm.lock-glslang.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the llvm.lock and glslang.lock keys are working? In Azure providing the filename means it will hash the file for the key. However, in https://github.com/actions/cache#creating-a-cache-key I see that they use this function: hashFiles('**/lockfiles')
to hash files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check
shell: bash | ||
run: .ci/setup-windows.sh | ||
|
||
- name: Export Variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should should be in a script file. Also, is it possible to do this step in bash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be done in bash, as for making it a file, I'm afraid that github.env
won't be resolved there, hence why it's a step.
.github/workflows/rpcs3.yml
Outdated
- name: Pack up build artifacts | ||
shell: bash | ||
run: | | ||
mkdir -p build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should mkdir $BUILD_ARTIFACTSTAGINGDIRECTORY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sorted
path: bin2 | ||
if-no-files-found: error | ||
|
||
- name: Upload artifacts (7z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problems with the linux version of this step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sorted
.github/workflows/rpcs3.yml
Outdated
run: | | ||
mkdir -p build | ||
.ci/deploy-windows.sh | ||
mkdir -p bin2/${{ env.BUILD_RAW }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using this bin2 folder instead of just using the existing bin folder for the zip artifact upload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bin2 is really only for zipped version artifact to not be double compressed. I'll remove it in favor of 7z
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zip removed
.github/workflows/rpcs3.yml
Outdated
mkdir -p bin2/${{ env.BUILD_RAW }} | ||
mv bin/* bin2/${{ env.BUILD_RAW }} | ||
- name: Upload artifacts (zip) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, we upload this seperate .zip because the .7z step gets double compressed? Perhaps we only deploy the 7z artifact in the case of a master build for upload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove zip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zip removed
Cache is supported on the penguin
Cold: https://github.com/illusion0001/rpcs3/actions/runs/10871089443/attempts/1
Warm: https://github.com/illusion0001/rpcs3/actions/runs/10871089443
Plans for later, build in clang and gcc on Windows and OSX