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

Set up the Swift version the extractor declares #1422

Merged
merged 22 commits into from
Dec 19, 2022

Conversation

angelapwen
Copy link
Contributor

@angelapwen angelapwen commented Dec 8, 2022

This PR changes the setup-swift Action to set up the version of Swift the extractor declares (except on Windows). It also adds the nightly-latest PR check back to a couple of PR checks, which were removed in #1412 to unblock the release.

Note that after this PR merges, we should remember to update the corresponding integration test within the CLI, and include the bumped setup-swift SHA (#1415) as well.

When the newest version of the CLI (that supports 5.7.1) makes it to latest and cached, we should be able to remove this logic and unconditionally setup 5.7.1, as future versions of the extractor should maintain backwards compatibility with prior Swift versions.

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
Copy link
Contributor Author

angelapwen commented Dec 8, 2022

cached and latest are failing (expected as they only support 5.7.0 and not 5.7.1).

I can conditionally setup 5.7.0 for latest, or we could wait to merge this PR until latest and cached reflect the last release which should support 5.7.1 🤔 (but this would mean that we're not testing nightly-latest until then).

@angelapwen angelapwen marked this pull request as ready for review December 8, 2022 15:20
@angelapwen angelapwen requested a review from a team as a code owner December 8, 2022 15:20
@henrymercer
Copy link
Contributor

For cached to reflect the latest release, we'd need to wait another 2 weeks or so.

I think perhaps while Swift is in beta, we should just test that the Swift extractor is compatible with the Swift version it declares:

codeql/experimental/swift/tools/*/extractor --version | awk '/version/ { print $3 }'

This mimics the internal tests, and while it's further away from customer setups, it avoids us having to update these tests every release while Swift is in beta.

@angelapwen angelapwen changed the title Unconditionally setup Swift 5.7.1 Set up the Swift version the extractor declares Dec 8, 2022
@angelapwen
Copy link
Contributor Author

For cached to reflect the latest release, we'd need to wait another 2 weeks or so.

I think perhaps while Swift is in beta, we should just test that the Swift extractor is compatible with the Swift version it declares:

codeql/experimental/swift/tools/*/extractor --version | awk '/version/ { print $3 }'

This mimics the internal tests, and while it's further away from customer setups, it avoids us having to update these tests every release while Swift is in beta.

Oh, that's a nice workaround. Updated 👍 (although I guess the internal tests should also be testing prior versions of Swift to catch problems similar to the one we just encountered)

@henrymercer
Copy link
Contributor

henrymercer commented Dec 8, 2022

Ah, that approach isn't very easy because we need to setup Swift before running codeql-action/init, but at that point CodeQL hasn't been downloaded yet, so we can't get the Swift version from the CodeQL distribution.

In that case, we might have to stop running this test on cached due to the bumpiness of which CodeQL version the cache contains, and manually specify which Swift versions correspond to each bundle.

Optionally, we could define a top-level setup-swift boolean in each generated workflow definition, and update sync.py to add the appropriate setup step to the Action.

@angelapwen
Copy link
Contributor Author

I've updated all 4 Swift-related PR checks to use 5.7.1, and stopped testing cached where I could but running into a couple issues:

  • Swift analysis using a custom build command on latest is failing because the latest version of CLI still is the version that requires 5.7.0 and not 5.7.1. This should cease to be an issue once our newest version goes into latest (and afterwards as well, as long as the extractor continues to support 5.7.1).
  • The multi-language autodetect check is failing on cached (and presumably latest) — even though I am no longer checking for the Swift database, it's still attempting to extract Swift because it's been automatically detected in the repository. I can't specify the languages to check because that defeats the purpose of this PR check (the autodetect).

Is it a viable solution to just wait until the newest CLI version that supports 5.7.1 makes it into latest and cached, and then merge this PR? It doesn't solve the longer-term problem of extractors not providing backwards compatibility with prior Swift versions, but hopefully this is not a common occurrence even in beta, and ideally should be tested in the CLI.

@angelapwen
Copy link
Contributor Author

Aditya suggested checking the Swift version from the extractor after the init step as a stopgap until the newest CLI makes it to latest. Will give that a try now.

@angelapwen
Copy link
Contributor Author

I think the above approach should work, but we are still getting this error — 

/home/runner/work/_temp/4bebb1c6-be8f-43cf-9dfd-75dc640e9229.sh: line 1: codeql/experimental/swift/tools/*/extractor: No such file or directory

Is this perhaps the wrong path, or has the extractor been cleaned up after the init step?

@angelapwen angelapwen force-pushed the swift-5.7.1 branch 3 times, most recently from 9c925fc to 2c0ca85 Compare December 16, 2022 01:19
@angelapwen angelapwen force-pushed the swift-5.7.1 branch 4 times, most recently from 8531eee to 3c9bc73 Compare December 16, 2022 17:58
@angelapwen angelapwen force-pushed the swift-5.7.1 branch 2 times, most recently from 79f629c to 4f1c8c5 Compare December 16, 2022 21:48
@henrymercer
Copy link
Contributor

henrymercer commented Dec 19, 2022

😮‍💨 I had an error that the new setup-swift step couldn't be found without running checkout, so I ran it again just before setup-swift but this overwrote the whole directory (I think). Reverted and trying to figure out why setup-swift can't be found when the checkout Action is run at the beginning of each check. But hit API rate limits again 🕐

Hmm, this is a little annoying. I think the problem is that the prepare-test step moves everything in the CodeQL Action repo apart from the multi-language test repository into action:

Run mkdir ../action
  mkdir ../action
  mv * .github ../action/
  mv ../action/tests/multi-language-repo/{*,.github} .
  mv ../action/.github/workflows .github

I expect that if we replace the call by ../action/.github/setup-swift then that should work.

@angelapwen
Copy link
Contributor Author

Hmm, this is a little annoying. I think the problem is that the prepare-test step moves everything in the CodeQL Action repo apart from the multi-language test repository into action:

Run mkdir ../action
  mkdir ../action
  mv * .github ../action/
  mv ../action/tests/multi-language-repo/{*,.github} .
  mv ../action/.github/workflows .github

I expect that if we replace the call by ../action/.github/setup-swift then that should work.

Thank you — that did it! That makes sense 😸

adityasharad
adityasharad previously approved these changes Dec 19, 2022
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Looks sensible, good work! Minor Bash suggestions.

.github/setup-swift/action.yml Outdated Show resolved Hide resolved
adityasharad
adityasharad previously approved these changes Dec 19, 2022
@angelapwen angelapwen enabled auto-merge (squash) December 19, 2022 18:33
@angelapwen
Copy link
Contributor Author

angelapwen commented Dec 19, 2022

Hm... the PR checks I changed don't seem to have run anymore. I'll close and reopen to see if this triggers them.

EDIT: Could be that the file path should be ./../action/.github/setup-swift rather than ../action/.github/setup-swift? Not sure why that would prevent the checks from running at all, but have updated for consistency.

@angelapwen angelapwen closed this Dec 19, 2022
auto-merge was automatically disabled December 19, 2022 20:00

Pull request was closed

@angelapwen angelapwen reopened this Dec 19, 2022
@angelapwen
Copy link
Contributor Author

Ok.. that seems to have done it 😆 re-requesting review (again), sorry!

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