Skip to content
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

fix #1032: add constants for file open #1078

Merged
merged 9 commits into from
Dec 13, 2018
Merged

Conversation

gms1
Copy link
Contributor

@gms1 gms1 commented Nov 10, 2018

This PR only propagates some missing constants for file open to JavaScript:

please see #1032 for a description of possible use cases

Flags For File Open Operations

@kewde
Copy link
Collaborator

kewde commented Dec 3, 2018

Looks good, thanks!
Seems like a CI failure, I'll restart it.

@kewde kewde closed this Dec 3, 2018
@kewde kewde reopened this Dec 3, 2018
@kewde
Copy link
Collaborator

kewde commented Dec 4, 2018

I've added a sample test for the shared database, to test a small aspect of it. Ideally we'd have more testing of the shared database but it seems like a more niche feature. On the other hand, I haven't seen tests specifically aimed at read-only databases either.

@kewde
Copy link
Collaborator

kewde commented Dec 4, 2018

@gms1 could you shine some light as to why the database creation is failing in the test I added? That seems to be the default way to open databases of different modes without the URI protocol.

@gms1
Copy link
Contributor Author

gms1 commented Dec 4, 2018

@kewde
many thanks!
something like that should work:

db = new sqlite3.Database('file:sqlite3orm',sqlite3.OPEN_SHAREDCACHE|sqlite3.SQLITE_OPEN_MEMORY,done);

or if compiled using SQLITE_USE_URI=1, or in combination with sqlite3.SQLITE_OPEN_URI, we can also use:

'file:sqlite3orm?mode=memory&cache=shared'

quoting from https://www.sqlite.org/inmemorydb.html
In-memory databases are allowed to use shared cache if they are opened using a URI filename. If the unadorned ":memory:" name is used to specify the in-memory database, then that database always has a private cache and is this only visible to the database connection that originally opened it.

@gms1
Copy link
Contributor Author

gms1 commented Dec 9, 2018

please review the added/modified tests

@gms1
Copy link
Contributor Author

gms1 commented Dec 9, 2018

hm all tests are passing during the first run at least, but only on linux, the "shared memory database" tests are failing during the second run: "SQLITE_ERROR: no such access mode: memory"
whats the difference between these two?

running these tests on my linux box works fine:

gms@orion:~/work/HOT/node-sqlite3 (add-open-flags)$ rm -rf node_modules/
gms@orion:~/work/HOT/node-sqlite3 (add-open-flags)$ npm install --build-from-source --sqlite=/usr --clang=1 &>/dev/null
gms@orion:~/work/HOT/node-sqlite3 (add-open-flags)$ npm run test &>/dev/null
gms@orion:~/work/HOT/node-sqlite3 (add-open-flags)$ echo $?
0

@kewde
Copy link
Collaborator

kewde commented Dec 10, 2018

The test pass for the SQLite3 package that has been bundled with the application.

Distributor ID:	Ubuntu
Description:	Ubuntu 12.04.5 LTS
Release:	12.04
Codename:	precise

The version in which it is supported is:
https://www.sqlite.org/changes.html#version_3_7_13

The external library for Ubuntu 12.04.5 LTS does not support it.
It has reached it's end of life support. Perhaps it's possible to only execute this test against a version sqlite3 that has the option? I prefer compiling the node-sqlite3 library on older versions of Linux so it has no library (libc, ...) compatibility issues.

@kewde kewde closed this Dec 10, 2018
@kewde kewde reopened this Dec 10, 2018
@kewde
Copy link
Collaborator

kewde commented Dec 10, 2018

@gms1
I think we can get the version of SQLite3 using select sqlite_version(); and then only run the test if the version supports it.

@gms1
Copy link
Contributor Author

gms1 commented Dec 11, 2018

@kewde
please review

@kewde kewde merged commit a8c34ea into TryGhost:master Dec 13, 2018
@kewde
Copy link
Collaborator

kewde commented Dec 13, 2018

Thank you for the contribution @gms1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants