Repository navigation
Conversation
1599deb to
0171ce3
Compare
bcaccd9 to
dd2f5ad
Compare
Also fix unescaped dot patterns in regex
There was a problem hiding this comment.
🟡 Changes recommended
Swift capability handling can still reject supported configurations, workflow conditions let cancellation bypass the Dependabot exclusion, and missing feature flags prevent key CI checks and regression tests from exercising the intended paths.
6 open issues
Enable unsupported-analysis checking in the generated workflow · New Parenthesize supplied conditions to preserve the exclusion · New Strengthen regression test with flags and a traced language · New Enable both unsupported-analysis flags in exemption tests · New Exclude Swift when the CLI supports all Swift platforms · New Apply Swift capability exemption to language selection
5 resolved since last review
2 files not reviewed
.github/workflows/__unsupported-macos.yml: Excluded by your content exclusion settingslib/entry-points.js: Excluded by your content exclusion settings
Review effort: Max
| if: "github.triggering_actor != 'dependabot[bot]'", | ||
| if: | ||
| jobSpecification.if !== undefined | ||
| ? `${baseCondition} && ${jobSpecification.if}` |
| logger, | ||
| }), | ||
| createTestConfig({ buildMode: BuildMode.None }), |
| platform: "darwin", | ||
| osRelease: "27.0.0", | ||
| env, | ||
| logger, |
| } | ||
|
|
||
| // If none of the languages are traced, then we can skip the compatibility checks below as well. | ||
| const tracedLanguages = await hasTracedLanguages(config, codeql); |
henrymercer
left a comment
There was a problem hiding this comment.
I'd like to dig deeper into the robustness of this method of checking the Xcode version. For instance, my machine has Xcode installed, but xcodeVersion would report "Xcode is probably not installed" because /private/var/db/xcode_select_link is missing. Also, Xcode is installed at /Applications/Xcode.app on my machine so even if Xcode was found, so the check would report that the path does not contain the expected pattern and we'd fail to determine the version number. I'm running macOS 27, and perhaps the behaviour would be different on a previous OS version, but I think we need to consider the variance in how Xcode might be installed if we're going to ship this to self-hosted runners.
Would you be able to share some discussion on why you chose this method? For instance, given we only care about checking whether the major version is >= 27, I'm curious how the check in the PR weighs up against e.g. xcrun --sdk macosx --show-sdk-version.
It might also be helpful to ship this more incrementally, e.g. we could ship the macOS 27 check as is in one PR, and we could ship the Xcode check as is but for hosted runners only in another. We could also add telemetry to measure whether the detection mechanism works in production. For instance, it might be useful to record if a Swift analysis completes successfully but the detection mechanism says Xcode is not installed.


Traced analysis is unsupported on macOS 27 (or newer) or with Xcode 27 (or newer). This PR adds a check for traced analyses which checks whether the macOS version is
< 27and the Xcode version is< 27.If either is
>= 27, then we report this issue to the user with a suitable diagnostic. I have also added a FF which controls whether this is just a warning or a fatal error which aborts the analysis.Further, there is a new
ToolsFeaturefor Swift which allows the CLI to indicate to us when it is ready to supportswifton all platforms. Once the CLI reports that it supports this, we skip all of the checks forswift.There are two new FFs: one which controls whether the new checks are performed and one which controls whether a failing check is a fatal error or not.
Notes for reviewers
checkRunnerSupportsTracedAnalysiswith respect to determining whether an analysis is traced.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist