Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Swift for private beta #1350

Merged
merged 37 commits into from
Nov 14, 2022
Merged

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Nov 7, 2022

This PR:

  • Updates the autobuild script path for Swift (it is expected that it lives in the experimental folder)
  • Adds PR checks to test Swift analysis with custom builds and autobuild with the expected CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT environment variable set; it looks like all other mechanisms are already in place (ie. Swift is already in the list of traced languages) so am testing that. I noticed it was missing in the linguist to language prefixes map so added it there. Note that custom builds are supported on ubuntu and MacOS but autobuild is only supported on MacOS for the time being.
  • Adds Swift interpretation and analysis timings to the status report on the CodeQL Action side. Note that we have not yet added the fields to the data warehouse, nor are we consuming it on github.com; this change will be a no-op until those changes are merged.

Adding Swift to the languages.test.ts file will have been done in #1322 so I didn't add it to this PR to prevent the would-be merge conflict from refactoring the test.

As a follow-up, we should add a PR check to test that Swift can be auto-detected once the corresponding CLI change makes it to a release.

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.

@angelapwen angelapwen changed the title Add PR check for Swift private beta Support Swift for private beta Nov 7, 2022
@angelapwen angelapwen marked this pull request as ready for review November 8, 2022 01:04
@angelapwen angelapwen requested a review from a team as a code owner November 8, 2022 01:04
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Looks good! Could we check that language autodetection works for Swift too by adding a some more assertions to the pr-checks/checks/multi-language-autodetect.yml test?

pr-checks/checks/test-swift.yml Outdated Show resolved Hide resolved
pr-checks/checks/test-swift.yml Outdated Show resolved Hide resolved
@angelapwen
Copy link
Contributor Author

angelapwen commented Nov 8, 2022

Looks good! Could we check that language autodetection works for Swift too by adding a some more assertions to the pr-checks/checks/multi-language-autodetect.yml test?

I actually made a separate Swift PR check because I saw that we deliberately made a separate Ruby check from the multilanguage test when Ruby was being rolled out for beta (https://github.com/github/codeql-action/pull/685/files). Do you think we should just combine them all into 1 check now, even though Swift is going to be in beta?

To clarify though, I believe the autodetection itself is being tested with the Ruby check, and the Swift check I added. They're both finding the Ruby/Swift code in the tests/multi-language-repo directory and detecting/building them with the build script appropriately; just in a separate test from the PR check that tests for all repos. I could rename it to test-swift-autodetect.yml to clarify.

@henrymercer
Copy link
Contributor

I actually made a separate Swift PR check because I saw that we deliberately made a separate Ruby check from the multilanguage test when Ruby was being rolled out for beta (https://github.com/github/codeql-action/pull/685/files).

The separate PR check makes sense, as we can validate that Swift isn't enabled when the experimental environment variable isn't set, because the autodetect languages test doesn't build a Swift DB.

I think the thing that's missing from both Ruby and Swift is a test that the autodetection mechanism works, i.e. if you don't pass a languages: input to the init Action, then we pick up on the Swift code in the repo when the experimental environment variable is set.

@angelapwen
Copy link
Contributor Author

angelapwen commented Nov 8, 2022

Ah, makes sense!

I made the following changes:

  • added a separate Ruby autodetect PR check (we'll be able to combine this with the multilanguage autodetect check when the experimental flag is deprecated)
  • added a separate Swift autodetect PR check (same comment as above). This test is currently failing until we make the appropriate changes on the CLI side to support autodetection of Swift.
  • added a separate Swift autobuild PR check. This test is currently failing with a Not implemented yet error even on nightly-latest version of CLI. I'm checking in with the Swift team to understand if it's expected to fail currently, or if it's a problem with the way I've set up the Swift test suite.

@angelapwen
Copy link
Contributor Author

angelapwen commented Nov 10, 2022

Autobuild and custom build PR checks now work 😄

Waiting on CLI side autodetection of Swift, and then hopefully that PR check will pass and this PR will be ready for review.

EDIT: We have a Check modules up to date PR check failing that I'm still trying to investigate (https://github.com/github/codeql-action/actions/runs/3432887603/jobs/5722624634) When I run the suggested npm ci && npm run removeNPMAbsolutePaths on my local MacOS machine there are no changes.

@AlexDenisov
Copy link

Waiting on CLI side autodetection of Swift, and then hopefully that PR check will pass and this PR will be ready for review.

What is "CLI side autodetection of Swift"?

@AlexDenisov
Copy link

Also, the failing job' log contains the following (modified by hand for readability):

/opt/hostedtoolcache/CodeQL/0.0.0-20221024/x64/codeql/codeql database init \
--db-cluster /home/runner/work/_temp/codeql_databases \
--source-root=/home/runner/work/codeql-action/codeql-action \
--language=javascript --language=python --language=java \
--language=csharp --language=ruby --language=cpp --language=go \
--begin-tracing --trace-process-name=Runner.Worker.exe \
--codescanning-config=/home/runner/work/_temp/user-config.yaml

note the absence of Swift in the list of languages.

@henrymercer
Copy link
Contributor

What is "CLI side autodetection of Swift"?

Language autodetection is about figuring out what languages to analyze when users don't provide a languages input to the init Action. Generally we advise leaving this blank so users will automatically benefit from support for new languages, as in the snippet below.

      # Initializes the CodeQL tools for scanning.
      - name: Initialize CodeQL
        uses: github/codeql-action/init@v2
        # Override language selection by uncommenting this and choosing your languages
        # with:
        #   languages: go, javascript, csharp, python, cpp, java

We are in the process of migrating some functionality from the CodeQL Action to the CLI, including language autodetection. As a result, the functionality is different between the CodeQL Action and the CLI. Language autodetection for Swift for instance works in the CodeQL Action but not the CLI.

My current thoughts about this is that Swift language autodetection not working in the CLI should not block merging this PR — we should disable it with CODEQL_PASS_CONFIG_TO_CLI=false in the PR check for now and add a comment explaining that it is not currently implemented.

@henrymercer
Copy link
Contributor

Looking at the other failing jobs, "PR Check - Swift analysis using a custom build command" is hanging on macOS due to the .NET certificate prompt. We need to add DOTNET_GENERATE_ASPNET_CERTIFICATE=false to all checks that run build.sh either manually or through the autobuilder to avoid this, and I think adding this environment variable to this check should make the check pass.

@henrymercer
Copy link
Contributor

Looks like we're not autodetecting Swift in the Action either — I think this is because the language auto-detection uses the languages on main. We could add the Swift code to multi-language-repo in a separate PR to introduce some Swift on main — not sure if there's a more elegant solution.

@angelapwen
Copy link
Contributor Author

angelapwen commented Nov 10, 2022

This PR should be ready to merge but is blocked on #1357 being merged first.

@angelapwen angelapwen marked this pull request as ready for review November 11, 2022 00:43
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for your diligence with the PR checks! I think we just need to merge in main to get the checked-in dependencies check passing.

@henrymercer henrymercer marked this pull request as draft November 14, 2022 17:45
@henrymercer henrymercer marked this pull request as ready for review November 14, 2022 17:45
@angelapwen angelapwen merged commit 0eacdb5 into main Nov 14, 2022
@angelapwen angelapwen deleted the angelapwen/swift-private-beta-env-var branch November 14, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants