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

Tweak GHA configs #526

Merged
merged 7 commits into from
Mar 31, 2024
Merged

Tweak GHA configs #526

merged 7 commits into from
Mar 31, 2024

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Mar 6, 2024

@Goooler Goooler changed the title Tweak GH configs Tweak GHA configs Mar 6, 2024
@@ -61,11 +62,11 @@ jobs:

analyze-python-scripts:
name: Python Scripts
runs-on: ubuntu-22.04
runs-on: ubuntu-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave 22.04 for stability reasons, we should better upgrade it manually once 24.04 is released and tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with:
name: macOS XCODE5 Artifacts
path: Binaries/*.zip
- name: Upload to Release
if: github.event_name == 'release'
uses: svenstaro/upload-release-action@e74ff71f7d8a4c4745b560a485cc5fdb9b5b999d
uses: svenstaro/upload-release-action@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a commit corresponding to their latest tag (release). We cannot entirely trust third-party repositories (non-GitHub) are not compromised and get their tags overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vit9696
Copy link
Contributor

vit9696 commented Mar 6, 2024

The changes look good to me, would you update other repos or is there no big need in them?

@Goooler
Copy link
Contributor Author

Goooler commented Mar 6, 2024

There are so many repos. I believe we can set up @renovate-bot to keep things up to date for them.

See https://github.com/apps/renovate

@mikebeaton
Copy link
Contributor

Hi - Just as a reminder, if we switch to ARM build runner we should probably remove this: https://github.com/acidanthera/OpenCorePkg/blob/master/build_oc.tool#L328-L336

(Or, if we don't remove it, we will be shipping AARCH64 versions of these couple of BaseTools, which is probably less useful and may confuse people.)

@Goooler
Copy link
Contributor Author

Goooler commented Mar 6, 2024

It could be addressed in a follow-up.

@vit9696
Copy link
Contributor

vit9696 commented Mar 6, 2024

Ouch, should we really remove that or rather make the tools FAT? In my opinion, should merge after we have a fix for it.

@mikebeaton
Copy link
Contributor

Okay, look for better fix subsequently, for the issue I raised, agreed.

@mikebeaton
Copy link
Contributor

mikebeaton commented Mar 7, 2024

@Goooler - Right, Vit's suggestion was to fix the issue I mentioned, first and then to merge your PR when that is done. So we will look at that, the plan is to do both after next release. Thank you.

@mikebeaton
Copy link
Contributor

mikebeaton commented Mar 31, 2024

I think this should be ready to merge, following acidanthera/audk@33a4015 .

Unfortunately the brew install shellcheck line from analyze.yml workflow is now failing on the M1 runner - presumably this will hit the Intel runner in due course too - because we are hitting this.

I am currently not managing to replicate the error, in order to fix it, using a local, non-container install of brew, even though brew, python3 and pip3 are all the same versions that are causing this to fail in the M1 runner.

Adding --break-system-packages is a possible, but I assume strongly non-preferred, work-around.

@mikebeaton
Copy link
Contributor

Fixed: ab5aeec

@mikebeaton mikebeaton merged commit ff7fd00 into acidanthera:master Mar 31, 2024
25 of 33 checks passed
@Goooler Goooler mentioned this pull request Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants