FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Output path to preload_tracer in env file by robertbrignull · Pull Request #360 · github/codeql-action · GitHub

/ codeql-action Public

Output path to preload_tracer in env file - #360

Merged
robertbrignull merged 7 commits into
mainfrom
robertbrignull/preload_tracer_env_var
Jan 13, 2021
ConversationCommits (7)ChecksFiles changed
Merged

Output path to preload_tracer in env file#360
robertbrignull merged 7 commits into
mainfrom
robertbrignull/preload_tracer_env_var

Conversation

robertbrignull commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

When building code on macos with SIP enabled it's necessary to prefix your build command with preload_tracer, so to make that easy we output a path to that executable in the env that you need to source. This means you should be able to run $CODEQL_PRELOAD_TRACER ./my-build-command.sh.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

robertbrignull commented Jan 11, 2021

Copy link
Copy Markdown
Contributor Author

@p0, thanks for your suggestion to use ${CODEQL_DIST}/tools/${CODEQL_ARCH}/preload_tracer.
I've used CODEQL_DIST in this but I'm not sure CODEQL_ARCH is available so I've hardcoded the path as ${CODEQL_DIST}/tools/osx64/preload_tracer. Is this ok? The code path is only executed on osx.

p0 commented Jan 11, 2021

Copy link
Copy Markdown

Yes, that would be safe enough.

aibaars commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

@p0, thanks for your suggestion to use ${CODEQL_DIST}/tools/${CODEQL_ARCH}/preload_tracer.
I've used CODEQL_DIST in this but I'm not sure CODEQL_ARCH is available so I've hardcoded the path as ${CODEQL_DIST}/tools/osx64/preload_tracer. Is this ok? The code path is only executed on osx.

The variable is called CODEQL_PLATFORM and should be available.

Why do we need to prefix things with the preload_tracer; wouldn't the codeql/tools/osx64/runner be enough?

robertbrignull commented Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

The variable is called CODEQL_PLATFORM and should be available.

Thanks. You're right. I've changed it to use that value.

aibaars reviewed Jan 12, 2021
View reviewed changes
Comment thread src/runner.ts Outdated
codeqlDist,
"tools",
codeqlPlatform,
"preload_tracer"

aibaars Jan 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@p0 Wouldn't it be better to use the runner instead. It is available for all platforms including windows so we simply populate an env variable like CODEQL_RUNNER for all platforms. The advantage is that things keep on working in case a user tries to run the CI job on various platforms (or copy pastes the "$CODEQL_PRELOAD_TRACER" ./build-command.sh command line into a linux job). This could lead to unexpected problems due to undefined or empty environment variables.

robertbrignull commented Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

I've switched over to using the runner and outputting the env var on all platforms. I'm going to write some tests for this.

aibaars reviewed Jan 12, 2021
View reviewed changes
Comment thread src/runner.ts Outdated
tracerConfig.env["CODEQL_DIST"],
"tools",
tracerConfig.env["CODEQL_PLATFORM"],
"runner"

aibaars Jan 12, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

On windows the binary is called runner.exe. I think it is fine to leave off the .exe extension on Windows, but you might want to add it just in case.

robertbrignull Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks. I was just looking at this. May as well add on the extension.

I also note there are runner-linux, runner-win.exe, and runner-osx. They appear to maybe be duplicates of the non-suffixed versions. Should these be ignored?

aibaars Jan 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think these are left-overs of the legacy tools.

robertbrignull commented Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

I've added the CODEQL_RUNNER var to the unit tests, and added usages of it to some of the integration tests. Unfortunately the unit tests don't run on windows (I only just discovered this) and they don't pass either so enabling them will be a bit of work. So for windows we're relying on the integration tests to show that things are working, but that's probably fine.

robertbrignull commented Jan 13, 2021

Copy link
Copy Markdown
Contributor Author

@aibaars or @adityasharad do you mind having another quick look over this and make sure I haven't done anything stupid since it was last reviewed?

aibaars approved these changes Jan 13, 2021
View reviewed changes

aibaars left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me.

robertbrignull merged commit 087e7a3 into main Jan 13, 2021
robertbrignull deleted the robertbrignull/preload_tracer_env_var branch January 13, 2021 10:15
github-actions Bot mentioned this pull request Jan 18, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

adityasharad adityasharad approved these changes
+1 more reviewer
aibaars aibaars approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL