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

autobuild: Update tests for C# on macOS #1149

Merged
merged 8 commits into from
Jul 27, 2022
Merged

Conversation

criemen
Copy link
Contributor

@criemen criemen commented Jul 21, 2022

  • Add a test for the autobuild action using C# on macOS. This should work under SIP as long as the tracer is set up correctly, thanks to Actions' own mechanism. Without the $CODEQL_RUNNER prefix, the tracer will not be injected on MacOS, as we need the runner to circumvent SIP.
  • Fix the corresponding test for the Runner's autobuild step, which was failing. This test needs the $CODEQL_RUNNER prefix since the tracer is not yet active in the environment. The CLR tracer previously mitigated this problem, but no longer works with dotnet 6 in the virtual environment.

After releasing this change, the required checks in the branch protection has to be updated.

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.

Without this, the tracer will not be injected on MacOS, as we need the
runner to circumvent SIP.
Also add a test that tests the autobuild-action to exercise this code path.
@criemen criemen marked this pull request as ready for review July 21, 2022 16:41
@criemen criemen requested a review from a team as a code owner July 21, 2022 16:41
@@ -0,0 +1,20 @@
name: "Autobuild "
description: "Tests that C# "
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line and the one above are incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and the file name is wrong either.

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.

LGTM. Minor suggestions only.

src/codeql.ts Outdated Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
pr-checks/checks/autobuild-macos-dotnet.yml Outdated Show resolved Hide resolved
@aibaars
Copy link
Collaborator

aibaars commented Jul 21, 2022

I wonder why this change is necessary, and wouldn't workflows with custom build commands also need a similar tweak? The actions/runner already inserts an intermediate process, so CODEQL_RUNNER should not be needed.

See also: actions/runner#416

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.

As discussed separately, we do not need to introduce the CODEQL_RUNNER prefix thanks to Actions' own protection mechanism, but we do want to keep the new PR check introduced here.

pr-checks/checks/autobuild-action.yml Show resolved Hide resolved
.github/workflows/__autobuild-macos-dotnet.yml Outdated Show resolved Hide resolved
Add the missing `$CODEQL_RUNNER` prefix to the autobuild command line.
This intermediate process works around System Integrity Protection,
allowing the tracer to start the C# extractor for the dotnet builds
within the autobuild process.

The test used to pass without this because the legacy CLR tracer bypassed SIP
while dotnet 5 was used on the Actions virtual environment.
Now that the virtual environment uses dotnet 6, the CLR tracer no longer works,
and we need to explicitly work around SIP.

This test will eventually be replaced by an internal integration test for the
equivalent functionality in the CLI. For now, this change makes the test
continue to pass.
@adityasharad adityasharad changed the title autobuild-action: Run autobuilders with $CODEQL_RUNNER set. autobuild: Update tests for C# on macOS Jul 25, 2022
src/codeql.ts Outdated Show resolved Hide resolved
Ensure that this succeeds even if the legacy CLR tracer is not enabled.
The combination of the regular tracer and the SIP workaround within Actions
should be sufficient for this to pass.
We do not need to prefix `$CODEQL_RUNNER` here on macOS to bypass SIP,
because we assume that the `init` step exported `DYLD_INSERT_LIBRARIES`
into the environment, which activates the Actions workaround for SIP.
See actions/runner#416.
@adityasharad adityasharad force-pushed the criemen/runner-autobuilders branch from 675310b to b4ff463 Compare July 25, 2022 22:03
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Thanks. Clearer now.

This test workflow does not source the environment from the init step,
so we need to manually read in the variable.
@adityasharad
Copy link
Contributor

@aeisenberg failing test is fixed: https://github.com/github/codeql-action/runs/7547434836?check_suite_focus=true. I added an explicit step to source the tracing environment (apparently the Runner's autobuild command wasn't sufficiently loading the environment, or it was too late in the process tree).

The failing check is a 503 in a test that I haven't touched; I say we ignore it and get this merged.

@aeisenberg
Copy link
Contributor

Check is passing now. i'd say it is ok to merge.

@adityasharad adityasharad merged commit bbc2e70 into main Jul 27, 2022
@adityasharad adityasharad deleted the criemen/runner-autobuilders branch July 27, 2022 20:16
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