JDBC_EXTENSIONS compilation option doesn't work anymore

[gotson]

Hi @michael-o ,

in #465 you added support for JDBC_EXTENSIONS as a compilation option, however it doesn't seem to work anymore, so that never works.

I tracked it down to this not doing anything, maybe because the source file changed and the perl line doesn't work as expected anymore?

If you have some time, do you want to have a look at this?

Thanks

[michael-o]

Sure, will check. It has been a while.

[michael-o]

OK, found the reason for back then and what needs to be done now to make this proper:
Good Old Times: SQLite did not have fancy math functions and alike. I didn't want to use the bundled SQLite because it is also used elsewhere, no duplicate administration. Since the tests use custom extensions I decided to add the pragra to detect at runtime whether this is an external build or bundled build.
Today:

• Many of these have been superseded or obsolete in the last couple of years and require a cleanup
• SQLite now comes with a bundle of math functions by default: https://www.sqlite.org/lang_mathfunc.html
• https://github.com/xerial/sqlite-jdbc/blob/master/src/main/ext/extension-functions.c needs to be stripped down to the functions SQLite does not provide
• This test is obsolete these days

I will check for the availability of reverse() and fix the code accordingly.

[michael-o]

I have done now the following (all on FreeBSD):

$ sqlite3 --version
3.37.2 2022-01-06 13:25:41 872ba256cbf61d9290b571c0e6d82a20c224ca3ad82971edc46b29818d5dalt1
$ git rm -r src/main/resources/org/sqlite/native
$ gmake native SQLITE_OBJ=/usr/local/lib/libsqlite3.so SQLITE_HEADER=/usr/local/include/sqlite3.h JAVAC=javac JAVA=java CC=cc
## This version now defitively does not contain `JDBC_EXTENSIONS`
$ mvn clean verify

output:

[INFO] Running org.sqlite.ExtensionTest
[WARNING] Tests run: 3, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 0.015 s - in org.sqlite.ExtensionTest

as expected

[INFO] Running org.sqlite.DBMetaDataTest
[ERROR] Tests run: 38, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.151 s <<< FAILURE! - in org.sqlite.DBMetaDataTest
[ERROR] version  Time elapsed: 0.005 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: db version ==> expected: <3.39.2> but was: <3.37.2>
        at org.sqlite.DBMetaDataTest.version(DBMetaDataTest.java:1360)

[INFO] Running org.sqlite.util.OSInfoTest
Aug 10, 2022 1:38:35 PM org.sqlite.util.OSInfoTest displayOSInfo
INFORMATION: Hardware name: amd64

Aug 10, 2022 1:38:35 PM org.sqlite.util.OSInfoTest displayOSInfo
INFORMATION: OS name: FreeBSD
Aug 10, 2022 1:38:35 PM org.sqlite.util.OSInfoTest displayOSInfo
INFORMATION: Architecture name: x86_64

as expected

[INFO] Running org.sqlite.MathFunctionsTest
[ERROR] Tests run: 27, Failures: 2, Errors: 4, Skipped: 0, Time elapsed: 0.015 s <<< FAILURE! - in org.sqlite.MathFunctionsTest
[ERROR] square  Time elapsed: 0 s  <<< ERROR!
org.sqlite.SQLiteException: [SQLITE_ERROR] SQL error or missing database (no such function: square)
        at org.sqlite.MathFunctionsTest.square(MathFunctionsTest.java:225)

[ERROR] cot  Time elapsed: 0.001 s  <<< ERROR!
org.sqlite.SQLiteException: [SQLITE_ERROR] SQL error or missing database (no such function: cot)
        at org.sqlite.MathFunctionsTest.cot(MathFunctionsTest.java:120)

[ERROR] log  Time elapsed: 0 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <0.693147180559945> but was: <0.30102999566398114>
        at org.sqlite.MathFunctionsTest.log(MathFunctionsTest.java:163)

[ERROR] atn2  Time elapsed: 0.001 s  <<< ERROR!
org.sqlite.SQLiteException: [SQLITE_ERROR] SQL error or missing database (no such function: atn2)
        at org.sqlite.MathFunctionsTest.atn2(MathFunctionsTest.java:80)

[ERROR] coth  Time elapsed: 0.001 s  <<< ERROR!
org.sqlite.SQLiteException: [SQLITE_ERROR] SQL error or missing database (no such function: coth)
        at org.sqlite.MathFunctionsTest.coth(MathFunctionsTest.java:128)

[ERROR] log10  Time elapsed: 0.001 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1.0> but was: <0.9999999999999999>
        at org.sqlite.MathFunctionsTest.log10(MathFunctionsTest.java:171)

well, someone did not check for JDBC_EXTENSIONS.

So this needs improvement.

Is that sufficient for you?

[michael-o]

Just made another check, the JDBC extension has not been added as a string. I will prepare a patch.

[gotson]

well, someone did not check for JDBC_EXTENSIONS.

If you're referring to my latest commit adding tests, indeed. That's how I found out about JDBC_EXTENSIONS.

Is that failing in your case because the extensions are not added ? Because it works well on my laptop and on CI, tests are passing, and even though JDBC_EXTENSIONS is not defined, the math functions are available.

• Many of these have been superseded or obsolete in the last couple of years and require a cleanup

Right, i will need to check on those. We removed the JSON one recently as it's bundled with, but i need to check for the SQLITE ones.

I don't know how the other ones work (HAVE_xxx), I will probably need to check the git log to understand the history.

• SQLite now comes with a bundle of math functions by default: https://www.sqlite.org/lang_mathfunc.html

Yes, i am working on that as part of #633, i have a branch on my fork where i did the changes already.

• https://github.com/xerial/sqlite-jdbc/blob/master/src/main/ext/extension-functions.c needs to be stripped down to the functions SQLite does not provide

I did this using #ifndef to make sure it remains compatible in case we change the compilation options. Will submit as a PR soon so it can be peer reviewed.

• This test is obsolete these days

Not until we close #633.

[gotson]

Is that failing in your case because the extensions are not added ? Because it works well on my laptop and on CI, tests are passing, and even though JDBC_EXTENSIONS is not defined, the math functions are available.

Oh i understand now, you compiled with your own SQLite, indeed then it would be missing the extension functions.

If possible it would be nice for the tests to run conditionally depending on the compilation options. For the recently added math tests i wanted to use JDBC_EXTENSIONS, hence this ticket. For the new math functions that would depend on SQLITE_ENABLE_MATH_FUNCTIONS.

The metadata one probably should check for SQLITE_ENABLE_COLUMN_METADATA.

[gotson]

Tomorrow i'll try to reproduce the tests errors you showed above, for that i need to compile the same way you do (and that would be a good occasion to tackle #753 !).

[gotson]

What i don't quite understand is why JDBC_EXTENSIONS was passed by modifying the source.

Couldn't we pass it in a similar fashion as that? -DJDBC_EXTENSIONS ?

[michael-o]

What i don't quite understand is why JDBC_EXTENSIONS was passed by modifying the source.

Couldn't we pass it in a similar fashion as that? -DJDBC_EXTENSIONS ?

No, since the pragmas are in a char* array which are read at runtime. As you can see in the sqlite3.c they are ifdefs for included or absent for features. A mere external define won't do.

[michael-o]

Is that failing in your case because the extensions are not added ? Because it works well on my laptop and on CI, tests are passing, and even though JDBC_EXTENSIONS is not defined, the math functions are available.

Oh i understand now, you compiled with your own SQLite, indeed then it would be missing the extension functions.

If possible it would be nice for the tests to run conditionally depending on the compilation options. For the recently added math tests i wanted to use JDBC_EXTENSIONS, hence this ticket. For the new math functions that would depend on SQLITE_ENABLE_MATH_FUNCTIONS.

The metadata one probably should check for SQLITE_ENABLE_COLUMN_METADATA.

Exactly, you got it. This is why I do compile this way on HP-UX to make sqlite-jdbc run:

 CPPFLAGS="$CPPFLAGS -DSQLITE_HAVE_ISNAN -DSQLITE_ENABLE_COLUMN_METADATA" \
  LDFLAGS="$LDFLAGS -Wl,+concatrpath -Wl,+b -Wl,$LIBDIR -L$LIBDIR" \
  $CONFIGURE --enable-readline

[gotson]

What i don't quite understand is why JDBC_EXTENSIONS was passed by modifying the source.
Couldn't we pass it in a similar fashion as that? -DJDBC_EXTENSIONS ?

No, since the pragmas are in a char* array which are read at runtime. As you can see in the sqlite3.c they are ifdefs for included or absent for features. A mere external define won't do.

Yeah i just tested that. It works if we want to use #ifdef but they wouldn't be returned by pragma compile_options.

[michael-o]

What i don't quite understand is why JDBC_EXTENSIONS was passed by modifying the source.
Couldn't we pass it in a similar fashion as that? -DJDBC_EXTENSIONS ?

No, since the pragmas are in a char* array which are read at runtime. As you can see in the sqlite3.c they are ifdefs for included or absent for features. A mere external define won't do.

Yeah i just tested that. It works if we want to use #ifdef but they wouldn't be returned by pragma compile_options.

Of course not, the upstream project would need to add that define for use. Then the perl magic would be redundant. you can try to convince them to add it.

[gotson]

Thanks a lot @michael-o, i can make the extension test pass with a make native and skip with a custom build like yours.

Tomorrow i will polish the recently added math tests.

[michael-o]

Thanks a lot @michael-o, i can make the extension test pass with a make native and skip with a custom build like yours.

Tomorrow i will polish the recently added math tests.

Sure, try to replicate my failures first. Mind that the -D in the Makefile really need a cleanup.

[gotson]

Thanks a lot @michael-o, i can make the extension test pass with a make native and skip with a custom build like yours.
Tomorrow i will polish the recently added math tests.

Sure, try to replicate my failures first. Mind that the -D in the Makefile really need a cleanup.

I've managed to replicate and committed some fixes, the tests should now run fine when using a provided amalgamation.

[michael-o]

Thanks a lot @michael-o, i can make the extension test pass with a make native and skip with a custom build like yours.
Tomorrow i will polish the recently added math tests.

Sure, try to replicate my failures first. Mind that the -D in the Makefile really need a cleanup.

I've managed to replicate and committed some fixes, the tests should now run fine when using a provided amalgamation.

Will check later as well.

[michael-o]

I can confirm that 97cc99d works with the procedure from here. Tests are properly skipped:

[WARNING] Tests run: 349, Failures: 0, Errors: 0, Skipped: 9