Issue 787: enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT #802

itizir · 2020-04-14T17:14:21Z

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.

coveralls · 2020-04-14T18:38:40Z

Coverage remained the same at 53.404% when pulling ad7dd67 on itizir:787/delete_limit into aa77c03 on mattn:master.

itizir · 2020-04-15T08:21:59Z

Dammit: libsqlite3 in osx doesn't support that feature, so the test fails in that environment.

mattn · 2020-04-16T05:47:17Z

I wonder the changes for sqlite-binding.c will be updated & reverted with go run upgrade/upgrade.go

itizir · 2020-04-16T14:15:15Z

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: #500 (comment)

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!

itizir · 2020-04-20T21:08:18Z

Hi again! Any thoughts about the change, then? :)

vzool · 2020-04-21T06:16:25Z

UP

mattn · 2020-05-02T15:41:49Z

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.

itizir · 2020-05-02T18:32:38Z

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! :)

mattn · 2020-05-07T11:56:45Z

Thanks. But please keep to write in Go since I want to run this script on Windows too.

itizir · 2020-05-07T12:22:41Z

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?

itizir · 2020-05-08T02:11:48Z

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).

mattn · 2020-05-14T14:27:03Z

upgrade/upgrade_windows.go

+)
+
+func buildAmalgamation(baseDir, buildFlags string) error {
+	args := []string{"/f", "Makefile.msc", "sqlite3.c"}


Please check "gcc" can be used at the first.

What command should be run if gcc is available? And how should I check if it is?
Sorry, I'm not developing on Windows, so I'm not sure of all the ways to build this, and what the typical toolchains would be.
The only thing I tried was installing Visual Studio and ActiveTcl, and then running the recommended nmake script. This worked for me. I didn't try MinGW or anything like that...

Ehm, I was still waiting for some clarifications here...

Still no feedback? :/

MinGW works mostly like UNIX.

Thanks for looking at it again!

So does running

$ sh configure $ make sqlite3.c

work indeed?

In which case should we get rid of upgrade_windows.go altogether?
Or should both ways of building be available, either picked based on some CLI flag, or by trying one and checking for 'executable not found' or something to decide whether it should fall back to the other way?

c:\msys64\usr\bin\bash.exe -c ./configure c:\msys64\usr\bin\make.exe sqlite3.c

Sounds good. I'll make windows and unix code symmetrical then, simply with default paths to bash and make different (and configurable). And will get rid of nmake, unless you think it's useful to leave it in there?

(Is the -c necessary, by the way? Not that it makes much difference... just curious. I will now invoke configure this way indeed, and run bash by default.)

Hey again! Any thoughts about the current implementation? Do you want me to do any more changes?

itizir · 2020-05-27T10:23:40Z

Hey @mattn! Are you still interested in this change, or should I just close the PR?

itizir · 2020-08-07T12:08:25Z

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...

codecov-commenter · 2020-09-02T22:47:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d9e2789) 46.72% compared to head (e5f99d7) 46.72%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #802   +/-   ##
=======================================
  Coverage   46.72%   46.72%           
=======================================
  Files          12       12           
  Lines        1526     1526           
=======================================
  Hits          713      713           
  Misses        671      671           
  Partials      142      142

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

itizir · 2020-09-02T22:53:38Z

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...

itizir · 2020-09-24T14:32:34Z

OK, I see you took a different direction. Updated this PR accordingly. Could you maybe try it out again at some point?

mattn · 2020-10-07T03:44:16Z

I tried to run new upgrade.go but nothing changed.

itizir · 2020-10-07T11:08:52Z

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 #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...

mattn · 2020-10-07T12:25:57Z

Now I exported upgrade package to https://github.com/mattn/go-sqlite3-upgrade

mattn · 2020-10-07T12:26:21Z

I run go-sqlite3-upgrade but nothing changes.

itizir · 2020-10-07T13:03:43Z

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?

mattn · 2020-10-07T13:08:13Z

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.

itizir · 2020-10-07T13:59:22Z

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.

mattn · 2020-10-07T16:13:42Z

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?

itizir · 2020-10-14T11:34:38Z

Ooops. Sorry. Could you please send the changes as same as this pull-request?

Did I put the changes in the right place? 🙂

mattn/go-sqlite3-upgrade#1

mholt · 2020-12-11T23:02:59Z

Subscribing; anything I can do to help this along?

itizir · 2021-01-05T13:16:17Z

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.

itizir · 2022-09-02T21:24:57Z

Pinging again: is this ever going to be of interest, or should I just close and scrap the PR?

mholt · 2022-09-02T22:02:06Z

@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.)

itizir · 2022-11-18T09:25:50Z

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...)

build amalgamation with SQLITE_ENABLE_UPDATE_DELETE_LIMIT upgrade: go back to go

itizir changed the title ~~#787: enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT~~ Issue 787: enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT Apr 14, 2020

itizir force-pushed the 787/delete_limit branch from 20be4be to b0f0357 Compare April 14, 2020 18:08

itizir force-pushed the 787/delete_limit branch from b0f0357 to 9955ad5 Compare April 15, 2020 08:39

itizir force-pushed the 787/delete_limit branch from 9955ad5 to bd27335 Compare May 2, 2020 18:27

mattn reviewed May 14, 2020

View reviewed changes

itizir force-pushed the 787/delete_limit branch from b760063 to fa4b879 Compare June 15, 2020 15:33

itizir force-pushed the 787/delete_limit branch from 53c4ba1 to ad7dd67 Compare June 30, 2020 16:59

itizir force-pushed the 787/delete_limit branch from ad7dd67 to 69ffa53 Compare September 2, 2020 22:41

itizir force-pushed the 787/delete_limit branch from 69ffa53 to b2839fc Compare September 24, 2020 14:27

itizir requested a review from mattn January 22, 2021 17:55

itizir mentioned this pull request Feb 16, 2021

Build amalgamation from full source to enable UPDATE_DELETE_LIMIT mattn/go-sqlite3-upgrade#1

Open

itizir force-pushed the 787/delete_limit branch from b2839fc to a1f0ed7 Compare May 25, 2021 10:05

powerjungle mentioned this pull request Feb 17, 2022

2.0.0 #598

Closed

20 tasks

aeneasr mentioned this pull request Jun 9, 2022

Remove expired data from the database, ISSUE-952 ory/kratos#2406

Merged

7 tasks

itizir force-pushed the 787/delete_limit branch from a1f0ed7 to defa76a Compare December 13, 2023 12:39

itizir and others added 6 commits December 14, 2023 14:39

rewrite upgrade in bash: build amalgamation

c2a074c

build amalgamation with SQLITE_ENABLE_UPDATE_DELETE_LIMIT upgrade: go back to go

test delete with limit

56bb1e2

feedback and tweaks

4c62eaf

sqlite.org now publishing sha3

6188a75

update upgrade script

1bac0a5

tweak on rebase

e5f99d7

itizir force-pushed the 787/delete_limit branch from defa76a to e5f99d7 Compare December 14, 2023 14:39

Issue 787: enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT #802

Are you sure you want to change the base?

Issue 787: enable SQLITE_ENABLE_UPDATE_DELETE_LIMIT #802

Conversation

itizir commented Apr 14, 2020 • edited

coveralls commented Apr 14, 2020 • edited

itizir commented Apr 15, 2020

mattn commented Apr 16, 2020

itizir commented Apr 16, 2020

itizir commented Apr 20, 2020

vzool commented Apr 21, 2020

mattn commented May 2, 2020

itizir commented May 2, 2020

mattn commented May 7, 2020

itizir commented May 7, 2020 • edited

itizir commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattn Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattn Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itizir commented May 27, 2020

itizir commented Aug 7, 2020

codecov-commenter commented Sep 2, 2020 • edited

Codecov Report

itizir commented Sep 2, 2020

itizir commented Sep 24, 2020

mattn commented Oct 7, 2020

itizir commented Oct 7, 2020

mattn commented Oct 7, 2020

mattn commented Oct 7, 2020

itizir commented Oct 7, 2020

mattn commented Oct 7, 2020

itizir commented Oct 7, 2020

mattn commented Oct 7, 2020

itizir commented Oct 14, 2020

mholt commented Dec 11, 2020

itizir commented Jan 5, 2021

itizir commented Sep 2, 2022

mholt commented Sep 2, 2022

itizir commented Nov 18, 2022 • edited

itizir commented Apr 14, 2020 •

edited

coveralls commented Apr 14, 2020 •

edited

itizir commented May 7, 2020 •

edited

mattn Jun 30, 2020 •

edited

mattn Jun 30, 2020 •

edited

codecov-commenter commented Sep 2, 2020 •

edited

itizir commented Nov 18, 2022 •

edited