go-sqlite3
go-sqlite3 copied to clipboard
mattn
Issue 787: enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT
Since rebuilding the amalgamation from scratch requires the whole ./configure && make thingy, running the upgrade script in go sadly won't do, so I rewrote it in bash.
~Oh, just though probably adding a unit test in the go package to check the DELETE with LIMIT works would be good.~ Done.
Should resolve #787.
Coverage remained the same at 53.404% when pulling ad7dd6705de0c5bc87e32087fc610125291e0673 on itizir:787/delete_limit into aa77c03e2fcb7ed283920d09a0fb6b873488b5be on mattn:master.
Dammit: libsqlite3 in osx doesn't support that feature, so the test fails in that environment.
I wonder the changes for sqlite-binding.c will be updated & reverted with go run upgrade/upgrade.go
Oh, is upgrade.go automatically run somewhere? Sorry, I'm not sure I understood what you meant there.
In this PR I suggest replacing the golang upgrade program with a bash script. It does basically the same as upgrade.go (only difference is the newline at the end of sqlite3ext.h: thought I'd keep that consistent...).
But it allows building the amalgamation with custom options, in particular SQLITE_ENABLE_UPDATE_DELETE_LIMIT. I've kept the commits with the switch to bash and with the addition of that flag separate: you should be able to check that without that flag you do get the same bindings as before.
I've now noticed rewriting the upgrade tool had indeed been suggested before: https://github.com/mattn/go-sqlite3/pull/500#issuecomment-349505086
If you don't feel comfortable merging in PRs with changes to the bindings directly, I could also remove this part of the change and let you run the updated upgrade script yourself afterwards. Up to you!
I don't want to update sqlite3-binding.c directly. So if you want to add this, please update upgrade/upgrad.go to enable you want.
Did I understand this right? You're happy with accepting my change to the upgrade script, but want to run it and commit the bindings changes yourself?
I've removed the changes to sqlite3-binding.c and sqlite3ext.h and added a t.Skip() to the 'delete with limit' test I've added.
Let me know if it's OK now! :)
Thanks. But please keep to write in Go since I want to run this script on Windows too.
I don't think that's possible, sadly. At least I couldn't think of a way to do so (I mean, even a go script would have to either call configure/make or nmake on windows).
EDIT: Hmm, it does look like invoking the build tools from go should work fine. There could then be a separate windows/unix implementation. Would the windows environment indeed have nmake available? I'm not sure I'll manage to test that it works all fine, but can try. Or if I do an initial implementation, would you check if it works as expected?
All right! Here's my attempt at going back to go. I ended up rewriting more of the original upgrade.go than I first thought I would; I hope it's ok.
It does run on Windows as well, provided the right tools are available (nmake + tcl interpreter).
Hey @mattn! Are you still interested in this change, or should I just close the PR?
It's been a while! Any chance you could have another look? :)
Also, maybe https://github.com/mattn/go-sqlite3/pull/828 should go in first: it's probably easier if I have to fix the rebase rather than the other way around...
Codecov Report
Merging #802 (a1f0ed7) into master (1157a42) will not change coverage. The diff coverage is
n/a.
@@ Coverage Diff @@
## master #802 +/- ##
=======================================
Coverage 46.80% 46.80%
=======================================
Files 11 11
Lines 1457 1457
=======================================
Hits 682 682
Misses 640 640
Partials 135 135
Continue to review full report at Codecov.
Legend - Click here to learn more
Ξ = absolute <relative> (impact),ΓΈ = not affected,? = missing dataPowered by Codecov. Last update 1157a42...a1f0ed7. Read the comment docs.
It's been a while! Any chance you could have another look? :)
Also, maybe #828 should go in first: it's probably easier if I have to fix the rebase rather than the other way around...
...anyway, so I did rebase on top of #828, assuming you might want that change indeed.
But then frustratingly sqlite decided to now give sha3 instead of sha1, so for a hash check I had to bring non-stdlib-packages back in (and committing the go.sum seems to break non-linux go 1.11 builds...). Let me know what you want to do about that...
OK, I see you took a different direction. Updated this PR accordingly. Could you maybe try it out again at some point?
I tried to run new upgrade.go but nothing changed.
π€ Very strange. You mean it ran with no errors, but the bindings didn't get edited at all??
As comparison, here's the output when I run it:
$ CGO_ENABLED=0 go run -tags=upgrade ./upgrade
Go-SQLite3 Upgrade Tool
Downloading https://www.sqlite.org/2020/sqlite-src-3330000.zip
Download successful and verified hash 2f705bdf760664ce605768af7c2b6b082b99fbb7a3446a3d296a513df15f22a8
Extracted sqlite source to sqlite-src-3330000/
Starting to generate amalgamation with CFLAGS: -DSQLITE_ENABLE_UPDATE_DELETE_LIMIT=1
Ran configure successfully
Ran make successfully
SQLite3 amalgamation built
Merging: ext/userauth/userauth.c into sqlite3-binding.c
Patched: sqlite3.c -> sqlite3-binding.c
Merging: ext/userauth/sqlite3userauth.h into sqlite3-binding.h
Patched: sqlite3.h -> sqlite3-binding.h
Patched: sqlite3ext.h -> sqlite3ext.h
Done patching amalgamation
Cleaning up source: deleting sqlite-src-3330000/
How do you deal with the ignore tag you introduced in https://github.com/mattn/go-sqlite3/pull/851 by the way? I didn't find any other way than deleting it from the upgrade files to be able to run the tool...
Now I exported upgrade package to https://github.com/mattn/go-sqlite3-upgrade
Now I exported upgrade package to https://github.com/mattn/go-sqlite3-upgrade I run go-sqlite3-upgrade but nothing changes.
Oh I see. Well I don't see a branch or something with my changes in the new repository. Did you just copy-paste the changes from my PR locally into your /go-sqlite3-upgrade?
Will you be deprecating the /upgrade folder from this repo entirely? Should I close this PR and make one against https://github.com/mattn/go-sqlite3-upgrade instead?
go-sqlite3-upgrade uses the code contained in this pull-request change set. I want to know what is updated and what is needed by this pull-request. Thanks.
I'm sorry, I'm still confused. :s The code I see in go-sqlite3-upgrade is the old one, not the one from this PR. That's why I was asking how you were running things.
The code I see in go-sqlite3-upgrade is the old one, not the one from this PR. That's why I was asking how you were running things.
Ooops. Sorry. Could you please send the changes as same as this pull-request?
Ooops. Sorry. Could you please send the changes as same as this pull-request?
Did I put the changes in the right place? π
https://github.com/mattn/go-sqlite3-upgrade/pull/1
Subscribing; anything I can do to help this along?
Good question... I can't answer it though! π
Well, maybe you could also double-check the change for sanity, give feedback if you have any; otherwise it's really up to mattn to find time to review and accept/reject the PR.
Pinging again: is this ever going to be of interest, or should I just close and scrap the PR?
@mattn A good amount of effort and discussion has been put into this PR -- would you be able to take a few minutes to review or approve it? (I don't have any expertise here, but it does seem to be thorough and the code looks pretty clean.)
Hi @rittneje! Sorry to ping you, but I thought I'd try to get the ball rolling again here, and you seem to be more actively managing the package now. Any thoughts on this? Would there still be interest in supporting this, or should I just shelve the change and continue maintaining a private fork indefinitely? (or I suppose until SQLite decides to include this in their published amalgamation by default...)