feat: detect invalid binaries and show warning #1124
Conversation
danez
added
the
type: feature
⏱ Benchmark resultsComparing with 02f248f largeDepsEsbuild: 6.5s⬆️ 1.55% increase vs. 02f248f LegendlargeDepsNft: 29.9s⬇️ 4.19% decrease vs. 02f248f LegendlargeDepsZisi: 44.8s⬇️ 3.90% decrease vs. 02f248f Legend |
src/runtimes/detect_runtime.ts
Outdated
| const binaryInfo = detect(buffer as Buffer) | ||
|
|
||
| if (!isValidFunctionBinary(binaryInfo)) { | ||
| console.warn(` |
There was a problem hiding this comment.
zip-it-and-ship-it can be invoked as a binary, which prints a JSON string to stdout. For this reason, we don't typically print any additional messages because that'll technically make the output incorrect. To get around this, we've been adding properties to the output and then reading them from Netlify Build and/or CLI and show warning/error messages accordingly.
This is something we should revisit, because I don't think we're actually using the binary version anywhere, but I still wanted to mention it.
There was a problem hiding this comment.
Okay, can you point me to something? I cannot find any other case where we supply warnings/errors in the json response.
There was a problem hiding this comment.
There was a problem hiding this comment.
The problem here is that I cannot do it this way because the function is never actually returned by zisi. It is simply ignored. Now I could change that and mark it as invalid/incompatible and handle the ignoring outside of zisi, but that is a pretty hefty breaking change for just a simple warning.
I would rather try to remove the bin of zisi altogether and keep the printing as is :)
Or not do the warning at all.
There was a problem hiding this comment.
Ah, I see. Agreed, let's not overcomplicate things! I think it's fine to ship this as is, and then look into removing the binary separately.
There was a problem hiding this comment.
I changed the logic slightly to simply not print the warning in cli mode.
| // CLI entry point | ||
| const runCli = async function () { | ||
| // @ts-expect-error TODO: `destFolder` and `srcFolder` are not being passed | ||
| // back from `parseArgs()`. | ||
| const { destFolder, srcFolder, ...options } = parseArgs() | ||
|
|
||
| try { | ||
| global.ZISI_CLI = true |
There was a problem hiding this comment.
Not necessarily, but the alternative would be to pass the flag through the complete function hierarchy. And as the binary of zisi seems it might be deprecated and removed at some point, this was by far the more straightforward solution.
Summary
This now detects binaries not built for Linux/amd64 and prints a warning in this case.
Fixes netlify/pillar-runtime/issues#231