Output environment file for Windows - #495
adityasharad merged 2 commits into
Conversation
Sorry, something went wrong.
| "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.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, can't hurt to have this target too for people debugging in other ways though 🙂
Sorry, something went wrong.
|
|
||
| - 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, done ✅
Sorry, something went wrong.
| (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.
Sorry, something went wrong.
There was a problem hiding this comment.
Aha, done. For reason I thought those files needed patching to stop the runner reading them again.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, done - except in the unit tests where we still test both regardless of platform.
Sorry, something went wrong.
hmakholm
left a comment
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.
Sorry, something went wrong.
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-windows test to unset the CODEQL_EXTRACTOR_CSHARP_ROOT variable before the build step. This would cause the test to fail if the environment file being produced was not correct.