Reinstate CodeQL on Mac if not signing the build #2569

softins · 2022-03-31T10:37:35Z

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

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

softins · 2022-03-31T10:42:03Z

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:

softins · 2022-03-31T11:12:59Z

And similarly after the final commit:

hoffie · 2022-04-04T21:25:52Z

.github/autobuild/mac.sh

@@ -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


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... ;))

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

hoffie · 2022-04-04T21:33:37Z

.github/workflows/autobuild.yml

        run:                        ${{ matrix.config.base_command }} setup
        env:
          JAMULUS_BUILD_VERSION:    ${{ needs.create_release.outputs.build_version }}
+          MACOS_CERTIFICATE:        ${{ secrets.MACOS_CERT}}


If we need to keep this at all, the missing space should be added

Suggested change

MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT}}

MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT }}

I just copied and pasted from the Build step. I see the space is missing there too, but I didn't notice before.

hoffie · 2022-04-06T09:33:53Z

Another thing to consider:
@ann0see already introduced a way to run CodeQL on the legacy build (downside: we have to take care of that when removing that, but there's an appropriate comment as a reminder now). If we make CodeQL conditional on the signing, this might block us from or need another reversal if we decide to sign all non-legacy Mac builds (maybe with temporary keys). While there is no concrete PR for that, I think it should be a goal in order to make the autobuild and release code paths as similar as possible. Additionally, Mac arm64/M1 builds seem to require workarounds as long as there is no signature at all.

softins · 2022-04-07T14:39:10Z

Another thing to consider: @ann0see already introduced a way to run CodeQL on the legacy build (downside: we have to take care of that when removing that, but there's an appropriate comment as a reminder now).

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.

.github/autobuild/mac.sh

ann0see · 2022-04-30T20:55:17Z

.github/autobuild/mac.sh

@@ -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.


Suggested change

# a return status of 1 from propagating to the script exit status.

# a return status of 1 from propagating to the script exit status.

Not sure why the spacing didn't work...

ann0see · 2023-05-03T20:56:48Z

Is this still an issue? I believe so?

I wouldn't consider this a final fix, especially if #2944 is accepted and merged.

softins · 2024-01-12T17:37:39Z

Closing this old PR, as the affected files have changed a lot since it was raised.

softins added 8 commits March 30, 2022 17:54

Test for codeql and signing conditions

870ac30

Next test

67e8a40

Output macos_signed from setup

fd04867

Give id to setup step and test its output

24cf550

More corrections

fde5a10

Only run CodeQL for Mac when not signing build

3b42770

Ignore return from prepare_signing during setup

5f3bd07

Adjust comments and remove test steps

3ac2f73

softins requested a review from hoffie March 31, 2022 11:13

hoffie reviewed Apr 4, 2022

View reviewed changes

Add some missing spaces

42bf935

Refactor the signing checks for Mac

c2c951a

Also rename the output for disabling codeql explicitly.

softins requested a review from hoffie April 7, 2022 14:54

pljones reviewed Apr 9, 2022

View reviewed changes

.github/autobuild/mac.sh Show resolved Hide resolved

ann0see reviewed Apr 30, 2022

View reviewed changes

ann0see added 2 commits April 30, 2022 22:55

Add spacing as requested

e554a71

Apply spacing fix again

60fc9c0

hoffie mentioned this pull request Aug 19, 2022

Autobuild: Limit build targets by default #2789

Merged

15 tasks

ann0see changed the base branch from master to main December 26, 2022 19:10

ann0see marked this pull request as draft July 1, 2023 19:43

pljones added the tooling Changes to the automated build system label Aug 19, 2023

softins closed this Jan 12, 2024

softins deleted the autobuild/softins-signing branch January 12, 2024 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reinstate CodeQL on Mac if not signing the build #2569

Reinstate CodeQL on Mac if not signing the build #2569

softins commented Mar 31, 2022 •

edited by ann0see

Loading

softins commented Mar 31, 2022

softins commented Mar 31, 2022

hoffie Apr 4, 2022 •

edited

Loading

softins Apr 4, 2022

hoffie Apr 4, 2022

softins Apr 4, 2022

hoffie commented Apr 6, 2022

softins commented Apr 7, 2022

ann0see Apr 30, 2022

ann0see Apr 30, 2022

ann0see commented May 3, 2023 •

edited

Loading

softins commented Jan 12, 2024

	MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT}}
	MACOS_CERTIFICATE: ${{ secrets.MACOS_CERT }}

	# a return status of 1 from propagating to the script exit status.
	# a return status of 1 from propagating to the script exit status.

Reinstate CodeQL on Mac if not signing the build #2569

Reinstate CodeQL on Mac if not signing the build #2569

Conversation

softins commented Mar 31, 2022 • edited by ann0see Loading

Checklist

softins commented Mar 31, 2022

softins commented Mar 31, 2022

hoffie Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

softins Apr 4, 2022

Choose a reason for hiding this comment

hoffie Apr 4, 2022

Choose a reason for hiding this comment

softins Apr 4, 2022

Choose a reason for hiding this comment

hoffie commented Apr 6, 2022

softins commented Apr 7, 2022

ann0see Apr 30, 2022

Choose a reason for hiding this comment

ann0see Apr 30, 2022

Choose a reason for hiding this comment

ann0see commented May 3, 2023 • edited Loading

softins commented Jan 12, 2024

softins commented Mar 31, 2022 •

edited by ann0see

Loading

hoffie Apr 4, 2022 •

edited

Loading

ann0see commented May 3, 2023 •

edited

Loading