Add Swift extractor to Code Scanning

[calumgrant]

No description provided.

[criemen]

Search for bazel in https://docs.github.com/en/enterprise-cloud@latest/code-security/codeql-cli/getting-started-with-the-codeql-cli/preparing-your-code-for-codeql-analysis for the options you'll want to use

[redsun82]

hmm the error is weird (and as you can see it's not really related to the options you removed). Might be a bug with abseil's own bazel configuration. Let me look.

[redsun82]

maybe try to update abseil to the latest version with

bazel_dep(name = "abseil-cpp", version = "20240116.1")

in MODULE.bazel. You can probably revert the last commit.

[calumgrant]

The build command worked once so I presumed that whatever I changed was related to those changes!

[redsun82]

The build command worked once so I presumed that whatever I changed was related to those changes!

Yeah, it probably does (although it shouldn't), I was just saying it's probably not related to those specific options but to the other ones (and a misbehaviour by abseil...). Honestly, I'd probably leave the flags aside if this proves to be too problematic. I think they are overkill, and probably very relevant for local database extraction (where you might have an already running untraced bazel server and a warm cache), but not very much so in a clean runner environment. What do you think @criemen?

[redsun82]

I could reproduce this locally with --spawn_strategy=local. It's pretty weird that a more lax sandboxing leads to that error, but it's probably a bug with abseil. I'll report it. In the meantime I'd suggest we go without the suggested flags.

[criemen]

Yeah, if this causes problems, you can drop everything besides --spawn_strategy=local, as long as we execute this on the standard ephemeral action runners.

[calumgrant]

I get the same problems with --spawn_strategy=local on a Linux machine. But we DO need this as we only seeing 16 compilation units in telemetry, which isn't right.

[redsun82]

hm, ok. I couldn't reproduce on a standalone abseil checkout, so something's off with the module import, I have an idea for a workaround, let me check

[criemen]

There's a bazel bug/design shortcoming where one header shadows another when the sandbox is disabled, thus leading to compile errors (this requires two headers of the same name). We ran into this at a customers, too. Not sure if this is what's happening with abseil, though.

[redsun82]

as a workaround, you can build @absl//absl/strings with the normal strategy, and then the extractor with the local one. Analysis will miss the absl strings library, but that could be good enough?

bazel build @absl//absl/strings
bazel run //swift:create-extractor-pack --spawn_strategy=local

[redsun82]

@calumgrant @criemen found a cleaner workaround, the issue is some weird interaction between a new feature of clang and bazel and local strategy.

https://maskray.me/blog/2022-09-25-layering-check-with-clang

adding --features=-layering_check should solve the issue 👍

[criemen]

Great find! In that case, I'd like to bring back all the options, and document why we disable the layering check here.

Edit: I'll be on PTO, please proceed without my input on this task.

[redsun82]

aaand there's a bug already open: bazelbuild/bazel#21592

[henrymercer]

Can't comment on the Bazel changes, but consider matrixing the analysis over languages (e.g. see the template here https://github.com/actions/starter-workflows/blob/main/code-scanning/codeql.yml). This is what we recommend to customers as it means the analyses can run in parallel, and if one fails it doesn't disrupt the other.

[adityasharad]

Adding both languages un-matrixed will also break our existing analysis categories and alert matching: since currently no category is specified it will fall back to the language.

[henrymercer]

I think when no category is specified we fall back to the workflow name, job name, and matrix parameters e.g. here .github/workflows/codeql-analysis.yml:CodeQL-Build. So actually matrixing the build would create new histories when it comes to alert matching. I think that's probably a price we're willing to pay, but it's less of a clear cut win.

[adityasharad]

Ah my mistake, wrong way round. Specifying the category when we do that is probably worthwhile then.

[calumgrant]

How do you feel about creating a second workflow file for C++ analysis of the Swift extractor? The build commands are completely different and independent.

[redsun82]

hmm, seems like a good idea to me! Then it shouldn't disrupt the current analysis state in any way

[calumgrant]

I created a second PR #16071 where the C++ analysis is performed in a separate workflow file. If this approach is better, we can close this one.

[calumgrant]

Closed in favour of #16071