Fixed various benchmark issues#935
Conversation
Benchmarks now hopefully work for pull requests from a fork Fixed comparison info not beeing emmitted correctly after TypeScriptToLua#878 Bump actions/github-script from 0.9.0 to v3 Moved create benchmark check script to standalone js file
| const create_benchmark_check = require(`${process.env.GITHUB_WORKSPACE}/commit/.github/scripts/create_benchmark_check.js`); | ||
| return create_benchmark_check({github, context, core}); | ||
| - name: Benchmark results | ||
| run: echo "${{steps.script-combine-results.outputs.result}}" | glow -s dark -w 120 - |
There was a problem hiding this comment.
Do we even need markdown as an intermediary encoding here? Would changing Lua to output a regular string with ANSI codes require too much changes?
There was a problem hiding this comment.
Definitely possible without markdown. But I think the tables would be quite a bit of work. Also terminal color handling is often different across platforms and emulators, I am not sure how much of an issue that is, but that might require some additional work.
| @@ -1,8 +1,11622 @@ | |||
| { | |||
| "name": "typescript-to-lua", | |||
| "version": "0.36.0", | |||
| "lockfileVersion": 1, | |||
| "lockfileVersion": 2, | |||
There was a problem hiding this comment.
FWIW that would mean that every contributor would have to use Node >=15, otherwise npm v6 would revert these lockfile changes. But not upgrading also isn't great, since then some contributors would have to rollback node. Alternatively we can wait for corepack to land, and lock npm to v6 even on newer node versions.
There was a problem hiding this comment.
Yeah I wasn't sure what to do about this one. I have a new system and therefore have a fresh install of node.
The changelog mentions that the new lockfile version is backwards compatible, but I have not been able to try it:
https://blog.npmjs.org/post/626173315965468672/npm-v7-series-beta-release-and-semver-major
package-lock.json is updated to a newer format, using "lockfileVersion": 2. This format is backwards-compatible with npm CLI versions using "lockfileVersion": 1, but older npm clients will print a warning about the version mismatch.
There was a problem hiding this comment.
Running npm install on v6 downgrades lockfile back to "lockfileVersion": 1 and removes new key, which I wouldn't really call backwards compatible. But I guess that's okay
|
TL;DR Tried to use a new GitHub feature to allow the benchmark check to be posted for fork PRs. But I had security concerns so instead the benchmark results are just emitted to the action log. My initial idea was to use GitHub actions new With
So why not use I am summarizing my thought process here, as a reference in case anyone thinks about using Instead the benchmark results are only posted to the log and no check is created. |
Co-authored-by: ark120202 <ark120202@gmail.com>
3 participants
Benchmarks now work for pull requests from a fork (at least partially see comment: #935 (comment))
Fixed comparison info not being emitted correctly after #878
Bump actions/github-script from 0.9.0 to v3
Moved create benchmark check script to standalone js file