-
Notifications
You must be signed in to change notification settings - Fork 19
CI improvements 20250918 #25
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
Conversation
.github/workflows/docker.yml
Outdated
| DISPATCH_TOKEN: ${{ secrets.DISPATCH_TOKEN }} | ||
| strategy: | ||
| matrix: | ||
| dest-repo: [ps2toolchain, ps2sdk, ps2sdk-ports, ps2-packer, ps2dev, gsKit, ps2link, ps2homebrew/ps2dev_forwarder] |
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 dispatching to all repos? Shouldn't we follow the same approach as before?
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 won't actually dispatch to all repos (see the exclude: key).
fjtrujy
left a comment
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.
Not sure if I follow your changes, do you want to do the same workflow for all the repositories?
scripts/001-binutils.sh
Outdated
| echo Macport base is $MACPORT_BASE | ||
| MACPORT_BASE=$(dirname $(port -q contents gmp|grep gmp.h)|sed s#/include##g) | ||
| printf 'Macport base is %s\n' "$MACPORT_BASE" | ||
| alias sed='gsed' |
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.
Is this really needed?
Unless it is necessary for ps2toolchain-ee, the IOP one compiled successfully without that. I know that for brew it was needed
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 is not.
| container: | ||
| image: ${{ matrix.os.container }} | ||
| options: ${{ matrix.os.container-options }} | ||
| timeout-minutes: 240 |
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 setting timeout? I think it doesn't make sense in our scenario
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 is in case the build hangs (e.g. stuck at prompt or stuck in infinite retry loop) it will allow to notice earlier. Github default runners have a maximum timeout of 6 hours, so I reduced it to 4 hours.
| shell: bash | ||
| packageManager: default | ||
| - machine: ubuntu-22.04-arm | ||
| container: "ubuntu:20.04" |
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 don't get why you use Docker inside of Ubuntu, and you actually use the OS version 20.04, which reduces compatibility, as the precompiled toolchain can't be used in modern versions.
I could understand using Docker for those OS GHA doesn't offer as VM, like for instance Fedora, but I don't get your current approach
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 actually increases compatibility, as stuff built on 20.04 also works on 22.04 and 24.04.
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.
But on the other hand, we loose the visibility of checking if the toolchain works with "actual & more used" Ubuntu version, which is 24.04...
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 it would be only for command line/server users then I would agree.
However, I think this helps more with desktop/graphical users as they may be lagging behind on the distribution version due to desktop environment, driver, or other related issues. This is probably the situation where more people would want to run the precompiled toolchain on their system.
If needed, in the future more configurations could be added.
Affirmative. |
3829060 to
8b2fa4b
Compare
fjtrujy
left a comment
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.
The thing that I don't like in general is the fact that now we have references to IOP, EE, GSKIT, and others in this DVP toolchain repo.
I know that the main reason was to keep files as few as possible, but honestly, I'm not sure if it brings more pros than cons.
Have you thought about getting some of these values from variables in the repo configuration? In this way we can keep the YML simpler and the same for every single repo, where the differences are on the "variables section" within the config of the repo
|
I'll put the export variables and the repository dispatch variables in a separate JSON file in this PR later. |
Sync changes with other repo Separate repository dispatch into another job Add ARM build
Merge changes from other repo Run in ubuntu 20.04 container for ubuntu runner Add 4 hour timeout use standardized printf instead of echo Only add gnu-sed to path if brew available Correct archive name Avoid Homebrew additional network stuff
4092326 to
3ad829a
Compare
|
Made the changes. Environment is now set up in a separate file and repository dispatch variables are now placed in a separate file. |
|
Ok, let's see how it is working! |
Notable improvements are running in a older Ubuntu container for compatibility improvements, and arm64 support for the Docker/OCI container