Output only to stdout in github actions

[underyx]

PR checklist:

• Purpose of the code is evident to future readers
• Tests included or PR comment includes a reproducible test plan
• Documentation is up-to-date
• A changelog entry was added to changelog.d for any user-facing change
• Change has no security implications (otherwise, ping security team)

If you're unsure on any of this, please see:

• Contribution guidelines!
• One of the more specific guides located here

[linear]

SC-607 stdout and stderr are mixed together in github actions

isaacs/github#1981

CleanShot 2023-02-09 at 11.33.382x.png

[emjin]

😭 This is interfering with how the benchmarks ingest results

[underyx]

😭 This is interfering with how the benchmarks ingest results

Could you link me to error logs or a line of code? Do benchmarks parse timing out of semgrep's text formatter output? If yes, can we switch it to use JSON output?

[emjin]

Oh we're also failing a bunch of the QA tests https://github.com/returntocorp/semgrep/actions/runs/4166516368/jobs/7211137547

Here's the benchmark run: https://github.com/returntocorp/semgrep/actions/runs/4166516368/jobs/7211136971

[emjin]

Do benchmarks parse timing out of semgrep's text formatter output? If yes, can we switch it to use JSON output?

No, but they are now stumped by the results header

2023-02-13 18:27:45,342 - /home/runner/work/semgrep/semgrep/cli/../perf/run-benchmarks - ERROR - Unable to decode the Semgrep result as JSON. Exiting.
******
b'                                                                                \n                                                                                \n\xe2\x95\xad\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x95\xae                                                                     \n\xe2\x94\x82 Results \xe2\x94\x82                                                                     \n\xe2\x95\xb0\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x94\x80\xe2\x95\xaf                                                                     \n                                                                                \n{"errors": [], "paths": {"_comment": "<add --verbose for a list of skipped paths>", "scanned": ["/home/runner/work/semgrep/semgrep/perf/bench/dummy/input/dummy/targets/hello.js",  ...

[nmote]

I guess it's a bit problematic to change this behavior whenever we are in GHA, since we run all of our tests in GHA. It seems like this will make all the snapshot tests behave differently when run locally vs when run in CI. This seems problematic, and I'm not sure how best to address it.

[underyx]

I guess it's a bit problematic to change this behavior whenever we are in GHA, since we run all of our tests in GHA. It seems like this will make all the snapshot tests behave differently when run locally vs when run in CI. This seems problematic, and I'm not sure how best to address it.

Our tests are already immune from this issue and have isolated env vars 👍

[underyx]

@emjin I think this is all we needed to do to fix benchmarks 2bb63ba

[nmote]

Cool, I will let @emjin finish the review then. Thanks for addressing this so quickly!

[emjin]

Benchmarks work! Unfortunately there are other tests failing. It looks like semgrep-core's stdout and stderr streams might also be mixed? And this is causing semgrep-core output to be incorrectly processed. Note that semgrep-proprietary isn't well tested so presumably has a similar problem

[underyx]

Oof. The issue is that semgrep-core has a Python preexec function which uses the same logger as the CLI.

This just happened to work before due to luck, but is conceptually wrong as there are very different rules of communication between core -> cli, than there are between cli -> user :D

[emjin]

Maybe we just have the offset output for launch, and we merge this after? This is seeming like a larger change since we're fixing how semgrep-core communicates, and I don't think the output being off is a blocker for launch

[emjin]

We went through a bunch of edge cases and think it looks good!