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

Fix Swift PR Checks on nightly-latest CLI #1696

Merged
merged 12 commits into from
May 24, 2023

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented May 23, 2023

Now that Swift analysis is on by default in the latest nightly CLI version, we are autodetecting Swift, but Swift is not set up in this workflow. This change fixes the Swift setup workflow to unblock CI by:

  • adding setup-swift workflow, and CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT for all versions except nightly-latest to the debug artifacts check
  • no longer setting the CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT flag for the nightly-latest CLI version, for any other check. This allows us to test the case where the flag is not set for nightly-latest as well.
  • updating the setup-swift workflow to only set up Swift if the codeql resolve languages includes Swift.

Once CLI v2.13.3 is released, we can stop setting the experimental flag for latest.

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.

Now that Swift analysis is on by default, we are autodetecting Swift, but Swift is not set up in this workflow. This change adds the Swift setup workflow.
We now can (and need to) to run these steps without the environment variable being set in the other PR checks.
@angelapwen angelapwen marked this pull request as ready for review May 23, 2023 18:47
@angelapwen angelapwen requested a review from a team as a code owner May 23, 2023 18:47
henrymercer
henrymercer previously approved these changes May 23, 2023
@angelapwen
Copy link
Contributor Author

Oh no, the step is now failing on past CLI versions. I'll simply pass the environment variable to the failing test then, and we can do The Right Thing when we remove the experimental flag for new versions.

We cannot remove the conditional in the `setup-swift` workflow because it will fail to find the Swift extractor in prior versions without the experimental flag set. For now we replace the conditional and pass the experimental flag to the failing PR check.
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.

How about:

  • Updating the autogenerated "Set environment variable for Swift enablement" step to stop setting CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT=true on nightly-latest
  • Updating .github/actions/setup-swift to run Swift setup if we're running on macOS/Linux and swift is included in codeql resolve languages (Note that based on this, Linux analysis requires 5.7.3)

This should handle the case where Swift is enabled by default in nightly-latest. Once 2.13.3 is released, we can remove latest from the list in "Set environment variable for Swift enablement".

@angelapwen angelapwen changed the title Add setup-swift workflow to debug artifacts PR check Fix Swift PR Checks on latest CLI May 23, 2023
@angelapwen angelapwen changed the title Fix Swift PR Checks on latest CLI Fix Swift PR Checks on nightly-latest CLI May 23, 2023
.github/actions/setup-swift/action.yml Outdated Show resolved Hide resolved
pr-checks/sync.py Show resolved Hide resolved
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.

Thanks for making the changes, I think it made sense to do the work of updating the PR checks now. I think the only remaining piece is to update multi-language-autodetect.yml to check language autodetection for Swift on nightly-latest too. This is the only remaining read of CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT I found in the codebase.

pr-checks/sync.py Show resolved Hide resolved
- uses: ./../action/.github/actions/setup-swift
if: matrix.version == 'nightly-latest'
with:
codeql-path: ${{ steps.init.outputs.codeql-path }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I think we just need to add id: init to the init step to make this work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... I was wondering what the failure was coming from. Thanks.

@angelapwen
Copy link
Contributor Author

angelapwen commented May 24, 2023

I think the only remaining piece is to update multi-language-autodetect.yml to check language autodetection for Swift on nightly-latest too. This is the only remaining read of CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT I found in the codebase.

Nice find — I've modified it to check on nightly-latest when the platform isn't Windows, in addition to when the experimental flag is set.

@angelapwen angelapwen enabled auto-merge (squash) May 24, 2023 16:58
@@ -66,7 +66,9 @@ steps:
fi

- name: Check language autodetect for Swift
if: env.CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT == 'true'
if: >-
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good, but I'm slightly surprised to see us using env.CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT == 'true' for Ruby too — that seems like a subset. I'm good with us addressing this separately though to unblock PR checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, I noticed this as well. I wonder if it's because we had previously set the CODEQL_ENABLE_EXPERIMENTAL_FEATURES_SWIFT variable globally, but then moved it individually to specific steps. I'll open a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR at #1699

# Specify 5.7.0, otherwise setup Action will default to latest minor version.
if [ $VERSION = "5.7" ]; then
VERSION="5.7.0"
if [ $SWIFT_EXTRACTOR_DIR = "null" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Not critical here, but for the future we should quote variables to prevent problems with word splitting.

shell: bash
env:
CODEQL_PATH: ${{ inputs.codeql-path }}
run: |
if [ $RUNNER_OS = "macOS" ]; then
if [[ $RUNNER_OS = "macOS" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we should probably pick one of (single brace, double brace) and (single equals, double equals) and run with it. I think for our purposes here they will behave the same though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree! I actually changed it to double brace here to match the other instances of $RUNNER_OS.

I think the bash I've seen so far in the workflow uses single equals for equality, but the Actions syntax outside of the bash scripts use double equals. That's what I've been following at least 😅

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.

3 participants