Review requested:

• @nodejs/security-wg

@lemire do you think the differences are adding in this PR mean the updater is not working correctly ?

It's not the first time I see weird behavior of accumulated updates in the same PR. I would recommend to change the workflows to use peter-evans/create-pull-request, as I did for V8 patch updates.

@mhdawson

I did not investigate the issue further but I am quite confident that issue is not upstream (in simdjson).

The way the updater works is that it pulls in release zip files. This should be fine. Yet, somehow, we end up with the wrong file !!!

Well. Not exactly. Both files have this line in it...

#define SIMDJSON_VERSION "3.11.6"

So we are pulling the right version.

Strangely, we copy two files (.h and .cpp) but only the .h seems broken.

The script is not exactly mysterious, this should work.

echo "Fetching simdjson source archive..."
curl -sL -o "$SIMDJSON_ZIP" "https://github.com/simdjson/simdjson/archive/refs/tags/$SIMDJSON_REF.zip"
unzip "$SIMDJSON_ZIP"
cd "simdjson-$NEW_VERSION"
mv "singleheader/simdjson.h" "$DEPS_DIR/simdjson"
mv "singleheader/simdjson.cpp" "$DEPS_DIR/simdjson"
mv "LICENSE" "$DEPS_DIR/simdjson"

@targos please see #56684

CI: https://ci.nodejs.org/job/node-test-pull-request/64576/

Could we close this PR, and fix the automation instead? If we land this in several commits, it means 8c0e4cb will be on main, which doesn't build, making bisecting needlessly harder. It would be also more useful to validate the automation works correctly.

If we land this in several commits, it means 8c0e4cb will be on main, which doesn't build, making bisecting needlessly harder. It would be also more useful to validate the automation works correctly.

I expected this PR to be squashed and merged. Isn't this so?

By default, it would use the first commit's message – and therefor land a commit with the wrong version. However, because it has the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label, it will not squash this time. But anyway, we can land it manually, but my point on "we should validate that the automation is actually fixed" still stands – no strong feelings though, feel free to disagree.

@aduh95

By default, it would use the first commit's message – and therefor land a commit with the wrong version. However, because it has the commit-queue-rebase label, it will not squash this time. But anyway, we can land it manually, but my point on "we should validate that the automation is actually fixed" still stands – no strong feelings though, feel free to disagree.

For a month, the automated update has been pushing broken code:

#56250

I have contributed something that might fix the updates:

#56684

If you ask me, I will just close this issue (the one we are on). Is that what you recommend?

Is that what you recommend?

Yes exactly, good to hear we do align (sorry if I wasn't clear, written communication is hard sometimes :D)

Given the discussion it's not clear if this PR is actually ready to move forward. I'm going to remove the author ready and commit-queue labels here just in case. If this is ready to land, please just follow up with a comment making that clear.

CI: https://ci.nodejs.org/job/node-test-pull-request/64780/

#56250 auto-update looks good

CI: https://ci.nodejs.org/job/node-test-pull-request/64804/

#56250 landed