Output environment file for Windows #495
Conversation
| @@ -6,6 +6,7 @@ | |||
| "scripts": { | |||
| "build": "tsc", | |||
| "test": "ava src/** --serial --verbose", | |||
| "test-debug": "ava src/** --serial --verbose --timeout=20m", | |||
There was a problem hiding this comment.
This is fine, but are you aware that you can debug a test from inside of vscode? See the .vscode/launch.json file.
There was a problem hiding this comment.
Yes, can't hurt to have this target too for people debugging in other ways though 🙂
.github/workflows/pr-checks.yml
Outdated
| @@ -465,7 +465,12 @@ jobs: | |||
|
|
|||
| - name: Build code | |||
| shell: powershell | |||
| # Note this step unsets the CODEQL_EXTRACTOR_CSHARP_ROOT to ensure that the .win32env file is read correctly | |||
There was a problem hiding this comment.
In what situations is this not being read correctly?
There was a problem hiding this comment.
Before this PR the file was not being produced at all, so I figured we need to modify an integration test to make sure it actually gets read when it is produced.
There was a problem hiding this comment.
Oh. I see, I misunderstood the comment and what you're doing here. By un-setting the CODEQL_EXTRACTOR_CSHARP_ROOT variable, you are ensuring that .win32env is being read correctly.
Could you update the comment to something like this?
| # Note this step unsets the CODEQL_EXTRACTOR_CSHARP_ROOT to ensure that the .win32env file is read correctly | |
| # Note we want to make sure that the .win32env file is read correctlyl, so we unset the CODEQL_EXTRACTOR_CSHARP_ROOT from the .sh file. |
There was a problem hiding this comment.
Sure, done ✅
.github/workflows/pr-checks.yml
Outdated
| (Get-Content ./codeql-runner/codeql-env.sh) | Select-String -pattern ".*CODEQL_EXTRACTOR_CSHARP_ROOT.*" -NotMatch | ? {$_.ToString().trim() -ne "" } | Set-Content ./codeql-runner/codeql-env.sh | ||
| $jsonEnv = Get-Content -Path ./codeql-runner/codeql-env.json -Raw | ConvertFrom-Json | ||
| $jsonEnv.PSObject.Properties.Remove('CODEQL_EXTRACTOR_CSHARP_ROOT') | ||
| $jsonEnv | ConvertTo-Json -Depth 1 | Out-File ./codeql-runner/codeql-env.json -Force |
There was a problem hiding this comment.
Could we make this simpler, by removing CODEQL_EXTRACTOR_CSHARP_ROOT from the environment after the Invoke-Expression step below? I think Clear-Variable is the PowerShell command.
The step below doesn't use the JSON file.
There was a problem hiding this comment.
Aha, done. For reason I thought those files needed patching to stop the runner reading them again.
src/tracer-config.ts
Outdated
| const envPath = `${spec}.environment`; | ||
| fs.writeFileSync(envPath, buffer); | ||
|
|
||
| // Prepare the content of the compound environment file on Windows |
There was a problem hiding this comment.
What do you think about doing this step only when we're on Windows, vs unconditionally?
There was a problem hiding this comment.
I would have a (weak!) preference to only write the file that corresponds to the current platform. I doubt it makes any concrete difference either way, but it feels most right to try to mimic what the actual preload_tracer does.
There was a problem hiding this comment.
Sure, done - except in the unit tests where we still test both regardless of platform.
edoardopirovano
force-pushed
the
windows-env-file
branch
from
71e7ea8
to
a5506d8
Compare
There was a problem hiding this comment.
LGTM.
The O(n²) buffer building makes me wince a bit, but (a) that's what the existing code does for Unix, (b) generally there are not that many environment variables to handle, and (c) the whole exercise ought to go away soon when we have multi-language support in the CLI anyway.
Currently, the environment file for the tracer is only being output in the Unix format. Thus, these variables are not being read by the tracer on Windows machines. This PR addresses this by also outputting the environment file in the Windows format.
I have also modified the
runner-analyze-csharp-windowstest to unset theCODEQL_EXTRACTOR_CSHARP_ROOTvariable before the build step. This would cause the test to fail if the environment file being produced was not correct.