Log warning if SIP is disabled and CLI version is < 2.15.1 #2261
Conversation
Prior to CLI v2.15.1, ARM runners were not supported by the build tracer. "macos-latest" is now an ARM runner, so we run these tests on the old CLIs on Intel runners instead.
angelapwen
force-pushed
the
angelapwen/arm-runner-fix
branch
from
645450c to
db8b7f5
Compare
Just so we can see all CLI versions that are failing on `macos-12`
angelapwen
force-pushed
the
angelapwen/arm-runner-fix
branch
from
e0223de to
b6e9f31
Compare
adityasharad
left a comment
adityasharad
left a comment
There was a problem hiding this comment.
Thanks - generally looks good but a couple of minor suggestions.
| !(await isSipEnabled(logger)) | ||
| ) { | ||
| logger.warning( | ||
| "CodeQL versions 2.15.0 and lower are not supported on MacOS ARM machines with System Integrity Protection (SIP) disabled.", |
There was a problem hiding this comment.
The warning mentions ARM, but you're not checking process.arch.
Either we can change the warning to say macOS in general with SIP disabled is not supported on <=2.15.0 (not strictly true, but I don't know if we fixed other relocation issues that would affect Intel),
or change the code above to check process.arch being arm or arm64.
There was a problem hiding this comment.
Ah yes, good point 👍 will change it to process.arch as I believe that's more accurate to the problem we were looking at in 2.15.1.
src/init-action.ts
Outdated
| // For CLI versions <2.15.1, build tracing caused errors in MacOS ARM machines with | ||
| // System Integrity Protection (SIP) disabled. | ||
| if ( | ||
| !(await codeQlVersionAbove(codeql, "2.15.1")) && |
There was a problem hiding this comment.
Minor, separate: I had to go look at the definition to remind myself whether this was > or >=. Perhaps we should rename it codeQlVersionAtLeast or similar.
There was a problem hiding this comment.
Good point, I always double check that it's using gte too. I've made the change
pr-checks/sync.py
Outdated
| (matrix.os == 'macos-latest' || | ||
| matrix.os == 'macos-12') && ( |
There was a problem hiding this comment.
Can we simplify this to just runner.os == 'macOS'?
angelapwen
force-pushed
the
angelapwen/arm-runner-fix
branch
from
ee744db to
492f7d6
Compare
src/init-action.ts
Outdated
| // System Integrity Protection (SIP) disabled. | ||
| if ( | ||
| !(await codeQlVersionAbove(codeql, "2.15.1")) && | ||
| process.platform === "darwin" && |
There was a problem hiding this comment.
This line needs to remain!
There was a problem hiding this comment.
Oh yes, Linux exists 😆
The
macos-latestimage is now ARM rather than Intel, so we need to change some of our PR Checks. The build tracer on CLI versions before v2.15.1 did not support the ARM machines where System Integrity Protection was disabled, which now includesmacos-latest.This change:
macos-12Intel runners for any checks where the CLI version is below v2.15.1Separately, the
macos-latestimage no longer supports Go on the path by default, so this PR addssetup-goto all PR checks analyzing Go.I've updated the
RequiredPR checks onmainto include the new ones, but have not updatedv2orv3yet.Merge / deployment checklist