FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Log warning if SIP is disabled and CLI version is < 2.15.1 by angelapwen · Pull Request #2261 · github/codeql-action · GitHub

/ codeql-action Public

Log warning if SIP is disabled and CLI version is < 2.15.1 - #2261

Merged
angelapwen merged 14 commits into
mainfrom
angelapwen/arm-runner-fix
Apr 25, 2024
ConversationCommits (14)ChecksFiles changed
Merged

Log warning if SIP is disabled and CLI version is < 2.15.1#2261
angelapwen merged 14 commits into
mainfrom
angelapwen/arm-runner-fix

Conversation

angelapwen commented Apr 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

The macos-latest image 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 includes macos-latest.

This change:

  • uses macos-12 Intel runners for any checks where the CLI version is below v2.15.1
  • logs a warning if SIP is disabled, and if the CLI version is below v2.15.1

Separately, the macos-latest image no longer supports Go on the path by default, so this PR adds setup-go to all PR checks analyzing Go.

I've updated the Required PR checks on main to include the new ones, but have not updated v2 or v3 yet.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

PR Checks: use macos-12 runners for CLI v. < 2.15.1
366c5f9 
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.
Log a warning if SIP is disabled and CLI is < 2.15.1
ab00339
Add changenote for SIP-disabled support on old CLI versions
db8b7f5
angelapwen force-pushed the angelapwen/arm-runner-fix branch from 645450c to db8b7f5 Compare April 25, 2024 17:35
Disable fail-fast (temporary)
b6e9f31 
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 April 25, 2024 18:26
angelapwen marked this pull request as ready for review April 25, 2024 20:41
angelapwen requested a review from a team as a code owner April 25, 2024 20:41
adityasharad reviewed Apr 25, 2024
View reviewed changes

adityasharad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks - generally looks good but a couple of minor suggestions.

Comment thread src/init-action.ts
!(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.",

adityasharad Apr 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

angelapwen Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread 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")) &&

adityasharad Apr 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

angelapwen Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good point, I always double check that it's using gte too. I've made the change

Comment thread pr-checks/sync.py Outdated
Comment on lines +92 to +93
(matrix.os == 'macos-latest' ||
matrix.os == 'macos-12') && (

adityasharad Apr 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we simplify this to just runner.os == 'macOS'?

angelapwen Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yep, done!

angelapwen force-pushed the angelapwen/arm-runner-fix branch from ee744db to 492f7d6 Compare April 25, 2024 21:58
adityasharad reviewed Apr 25, 2024
View reviewed changes
Comment thread src/init-action.ts Outdated
// System Integrity Protection (SIP) disabled.
if (
!(await codeQlVersionAbove(codeql, "2.15.1")) &&
process.platform === "darwin" &&

adityasharad Apr 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This line needs to remain!

angelapwen Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh yes, Linux exists 😆

angelapwen enabled auto-merge (squash) April 25, 2024 22:07
angelapwen merged commit ac2f82a into main Apr 25, 2024
angelapwen deleted the angelapwen/arm-runner-fix branch April 25, 2024 22:20
This was referenced Apr 25, 2024
Merged
Merged
angelapwen mentioned this pull request May 17, 2024
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

adityasharad adityasharad approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL