-
Notifications
You must be signed in to change notification settings - Fork 30.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deps: update simdjson to 3.11.6 #56663
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
@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 |
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" |
Could we close this PR, and fix the automation instead? If we land this in several commits, it means 8c0e4cb will be on |
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
|
For a month, the automated update has been pushing broken code: I have contributed something that might fix the updates: If you ask me, I will just close this issue (the one we are on). 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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
#56250 auto-update looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
automated PR is fixed
I pulled the automated update branch #56250 and I ran the manual update script
./dep_updaters/update-simdjson.sh
after disabling this code so that it will still run despite being at the version 3.11.6 already:Doing so should not change anything: if you update to the current version, you should simply redownload the same files.
I get this pull request. Observe how the result differs: it should not differ.