Reinstate CodeQL on Mac if not signing the build #2569
Conversation
|
After reviews, I will squash to a single commit before merging. I have left the original commits for review, to show the process of arriving at the solution. Before the final commit, I pushed to both emlynmac and softins, to compare the runs: |
|
And similarly after the final commit: |
.github/autobuild/mac.sh
Outdated
| @@ -73,6 +73,9 @@ pass_artifact_to_job() { | |||
| case "${1:-}" in | |||
| setup) | |||
| setup | |||
| # set up the macos_signed output if needed, but prevent | |||
| # a return status of 1 from propagating to the script exit status. | |||
| prepare_signing || true | |||
There was a problem hiding this comment.
Hmm, so we would now be running those steps twice?
Maybe there's some nice way to save that state and skip the second run somehow. Either pass back the macos_signed output variable as an environment variable or touch some kind of file.
(Note: see next comment before doing investigating this... ;))
There was a problem hiding this comment.
Well in fact, the only parts of prepare_signing that are needed in the setup phase are the initial tests and the setting of the output variable (so that it can be tested in the workflow). So we could make a different function check_signing that just does those parts, and maybe even call it from prepare_signing to reactor those parts out.
.github/workflows/autobuild.yml
Outdated
| run: ${{ matrix.config.base_command }} setup | ||
| env: | ||
| JAMULUS_BUILD_VERSION: ${{ needs.create_release.outputs.build_version }} | ||
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT}} |
There was a problem hiding this comment.
If we need to keep this at all, the missing space should be added
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT}} | |
| MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT }} |
There was a problem hiding this comment.
I just copied and pasted from the Build step. I see the space is missing there too, but I didn't notice before.
|
Another thing to consider: |
This PR turns off the running of CodeQL on Legacy and removes the comment, as it would no longer be needed. I can't see any reason to run CodeQL on both Mac builds. |
Also rename the output for disabling codeql explicitly.
| @@ -73,6 +81,9 @@ pass_artifact_to_job() { | |||
| case "${1:-}" in | |||
| setup) | |||
| setup | |||
| # check whether signing will be used and prevent | |||
| # a return status of 1 from propagating to the script exit status. | |||
There was a problem hiding this comment.
| # a return status of 1 from propagating to the script exit status. | |
| # a return status of 1 from propagating to the script exit status. |
There was a problem hiding this comment.
Not sure why the spacing didn't work...
|
Is this still an issue? I believe so? I wouldn't consider this a final fix, especially if #2944 is accepted and merged. |
|
Closing this old PR, as the affected files have changed a lot since it was raised. |
Short description of changes
Moves the CodeQL from Mac Legacy back to Mac, and makes it conditional on not doing a signed build.
CHANGELOG: Mac: Make Github workflow skip CodeQL when doing a signed build, to avoid hanging.
Context: Fixes an issue?
Replaces #2564 and #2566, and solves the issues encountered when working on #2563.
Does this change need documentation? What needs to be documented and how?
Probably not
Status of this Pull Request
Ready for review
What is missing until this pull request can be merged?
Review
Checklist