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

Exclude the temporary directory from scanning. #236

Merged
merged 5 commits into from
Oct 5, 2020
Merged

Conversation

chrisgavin
Copy link
Contributor

This fixes the case where the CodeQL Runner is downloaded on-the-fly to the source directory of the repository being analyzed. It gets copied to the toolcache which is outside of the working directory, but still remains in the repository directory too. We could delete it from the repository directory after copying it to the toolcache but a more robust solution seems like it would just be to ignore the temporary directory completely.

It's not ideal that people download the runner in this way because even compiled languages could take issue with unexpected files in the repository but it seems like people do so we should handle it as well as we can.

Merge / deployment checklist

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

@chrisgavin chrisgavin force-pushed the ignore-temp-dir branch 5 times, most recently from b7a2046 to 6f152ce Compare September 28, 2020 20:27
@chrisgavin chrisgavin changed the title Always exclude the temporary directory from scanning. Exclude the temporary directory from scanning. Sep 28, 2020
@chrisgavin chrisgavin marked this pull request as ready for review September 28, 2020 21:09
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Looks generally good to me with one comment. I want to test it to confirm my understanding of the behaviour. Should get that testing done today or tomorrow.

);
pathsIgnore = pathsIgnore.concat(config.tempDir);
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 you want to add tempRelativeToWorking here. These paths are relative to the cwd.

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 okay. At least with the JavaScript extractor I had no trouble with an absolute path, but if we're using relative elsewhere that does seem safer in case other extractors are less forgiving.

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 the python extractor is more picky and only works with relative paths. That's what I remember. There's an issue on the CodeQL side to make that behaviour more consistent.

Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

I've reproduced the bug with the codeql-runner-linux executable not in the cwd or the directory of the repository. As noted earlier the issue is with the temporary directory being in the current directory. By default the temp directory is ./codeql-runner relative to the cwd, i.e. where the runner executable was run from but not necessarily where it is located.

Something else I realised when testing this is this bug won't happen every time. The files being detected are those from the downloaded and unpacked bundle. This only happens if we don't find the bundle in the toolcache, and hence we download a new version to the temp dir and copy it to the toolcache. So on the next run the temp dir is wiped, but the bundle is not re-downloaded because we already have one in the toolcache that is no wiped.

You could get a similar problem if the toolcache is located in the cwd. The toolcache by default is created in the home directory, so this could happen if for some reason analysis is being run directly from the home directory.

On the plus side, I've tested using a runner built from this PR and it wokred. I tested it for javascript and python.

src/analysis-paths.ts Outdated Show resolved Hide resolved
src/analysis-paths.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I've done another quick test of this to make sure. Everything looks good and it's working perfectly.

@chrisgavin
Copy link
Contributor Author

No worries! I'm glad it all works.

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.

2 participants