Skip Menu |

This queue is for tickets about the DBD-SQLite CPAN distribution.

Report information
The Basics
Id: 111942
Status: resolved
Priority: 0/
Queue: DBD-SQLite

People
Owner: Nobody in particular
Requestors: vlmarek [...] volny.cz
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.48
Fixed in: (no value)



Subject: Support for sqlite3 missing column metadata feature
Hi, Second issue I have found while packing DBD::SQLite to Solaris. We ship sqlite without column metadata feature. That causes two issues a) DBD::SQLite can't be compiled, because of the -DSQLITE_ENABLE_COLUMN_METADATA parameter used during build. b) tests against this feature are failing. The attached patch tries to detect such a condition and act accordingly. The only way I can think off how to detect the compilation options in Makefile.PL is to run sqlite binary and ask. It tries to find the binary in the same way how sqlite.h is located. If the detection fails, Makefile.PL should behave as before - use -DSQLITE_ENABLE_COLUMN_METADATA compile option. I have tested this with sqlite library on Solaris which does not have column metadata compiled in. Also I have tested building on Solaris with the builtin sqlite.c.
Subject: 03_no_column_metadata.patch
--- DBD-SQLite-1.48/Makefile.PL 2016-02-11 00:39:32.919514864 -0800 +++ DBD-SQLite-1.48/Makefile.PL 2016-02-11 00:39:23.769957270 -0800 @@ -128,14 +128,15 @@ SCOPE: { # the system SQLite. We expect that anyone sophisticated enough to use # a system sqlite is also sophisticated enough to have a patching system # that can change the if ( 0 ) to if ( 1 ) -my ($sqlite_local, $sqlite_base, $sqlite_lib, $sqlite_inc); -if ( 1 ) { +my ($sqlite_local, $sqlite_base, $sqlite_lib, $sqlite_inc, $sqlite_bin, $compile_opts); +if ( 1 ) { require File::Spec; if ( $sqlite_base = (grep(/SQLITE_LOCATION=.*/, @ARGV))[0] ) { $sqlite_base =~ /=(.*)/; $sqlite_base = $1; $sqlite_lib = File::Spec->catdir( $sqlite_base, 'lib' ); $sqlite_inc = File::Spec->catdir( $sqlite_base, 'include' ); + $sqlite_bin = File::Spec->catdir( $sqlite_base, 'bin' ); } if ( $sqlite_local = (grep(/USE_LOCAL_SQLITE=.*/, @ARGV))[0] ) { $sqlite_local =~ /=(.*)/; @@ -144,6 +145,7 @@ if ( 0 ) { # Keep these from making into CFLAGS/LDFLAGS undef $sqlite_lib; undef $sqlite_inc; + undef $sqlite_bin; } } @@ -183,8 +185,32 @@ if ( 0 ) { $sqlite_local = 1; undef $sqlite_lib; undef $sqlite_inc; + undef $sqlite_bin; } else { print "Looks good\n" if $ENV{AUTOMATED_TESTING}; + + use FileHandle; + use IPC::Open2; + if (defined $sqlite_bin) { + $sqlite_bin = File::Spec->catdir( $sqlite_bin, 'sqlite3' ); + } else { + $sqlite_bin = 'sqlite3'; + } + if ( open2(*Reader, *Writer, "$sqlite_bin" ) ) { + print Writer "WITH opts(n, opt) AS ( + VALUES(0, NULL) + UNION ALL + SELECT n + 1, sqlite_compileoption_get(n) + FROM opts + WHERE sqlite_compileoption_get(n) IS NOT NULL + ) + SELECT opt + FROM opts;"; + close Writer; # flush + $compile_opts = join '', <Reader>; + } else { + warn "Can't execute '$sqlite_bin':$!"; + } } } } else { @@ -223,12 +249,33 @@ my @CC_DEFINE = ( '-DSQLITE_ENABLE_FTS4', # for sqlite >= 3.7.4 '-DSQLITE_ENABLE_FTS3_PARENTHESIS', # for sqlite >= 3.6.10 '-DSQLITE_ENABLE_RTREE', # for sqlite >= 3.6.10 - '-DSQLITE_ENABLE_COLUMN_METADATA', '-DSQLITE_ENABLE_STAT3', # for sqlite >= 3.7.9 '-DSQLITE_ENABLE_STAT4', # for sqlite >= 3.8.3.1 '-DNDEBUG=1', ); +# If we were able to extract compile options from sqlite3 binary and it +# contains ENABLE_COLUMN_METADATA, enable this option during our compilation +# too. If we were not able to extract the compilation options for some reason, +# enable the option too, as this is long time default behavior of this +# configure script. If we build local sqlite, enable the option too. +my $reason; +if ( $sqlite_local ) { + $reason = 'we are building against local sqlite3'; +} elsif ( not defined $compile_opts ) { + $reason = "we can't extract compile options out of existing sqlite3 binary"; +} elsif ( $compile_opts =~ m/^ENABLE_COLUMN_METADATA$/m ) { + $reason = "of it's support in existing sqlite3 binary"; +} + +print "Column metadata are "; +if ( $reason ) { + push @CC_DEFINE, '-DSQLITE_ENABLE_COLUMN_METADATA'; + print "enabled because $reason.\n"; +} else { + print "disabled.\n"; +} + if (DEVELOPER_ONLY) { # for sqlite >= 3.8.8 push @CC_DEFINE, '-DSQLITE_ENABLE_API_ARMOR'; --- DBD-SQLite-1.48/t/51_table_column_metadata.t 2016-02-11 00:48:50.345243742 -0800 +++ DBD-SQLite-1.48/t/51_table_column_metadata.t 2016-02-11 00:48:26.452736717 -0800 @@ -10,7 +10,17 @@ use t::lib::Test qw/connect_ok @CALL_FUN use Test::More; use Test::NoWarnings; -plan tests => 16 * @CALL_FUNCS + 1; +my $num_tests; +my $column_metadata; # defined when columnt metadata feature is available + +if (grep /ENABLE_COLUMN_METADATA/, DBD::SQLite::compile_options()) { + $num_tests = 16; + $column_metadata = 1; +} else { + $num_tests = 12; +} + +plan tests => $num_tests * @CALL_FUNCS + 1; for my $call_func (@CALL_FUNCS) { my $dbh = connect_ok(RaiseError => 1); $dbh->do('create table foo (id integer primary key autoincrement, "name space", unique_col integer unique)'); @@ -18,9 +28,11 @@ for my $call_func (@CALL_FUNCS) { { my $data = $dbh->$call_func(undef, 'foo', 'id', 'table_column_metadata'); ok $data && ref $data eq ref {}, "got a metadata"; - ok $data->{auto_increment}, "id is auto incremental"; - is $data->{data_type} => 'integer', "data type is correct"; - ok $data->{primary}, "id is a primary key"; + if ( $column_metadata ) { + ok $data->{auto_increment}, "id is auto incremental"; + is $data->{data_type} => 'integer', "data type is correct"; + ok $data->{primary}, "id is a primary key"; + } ok !$data->{not_null}, "id is not null"; } @@ -28,7 +40,9 @@ for my $call_func (@CALL_FUNCS) { my $data = $dbh->$call_func(undef, 'foo', 'name space', 'table_column_metadata'); ok $data && ref $data eq ref {}, "got a metadata"; ok !$data->{auto_increment}, "name space is not auto incremental"; - is $data->{data_type} => undef, "data type is not defined"; + if ( $column_metadata ) { + is $data->{data_type} => undef, "data type is not defined"; + } ok !$data->{primary}, "name space is not a primary key"; ok !$data->{not_null}, "name space is not null"; }
From: vlmarek [...] volny.cz
Uh, oh, sorry, the patch won't probably apply cleanly, it has enabled linking to system sqlite3.so in Makefile.pl. Please change -if (1) { +if (1) { into -if (0) { +if (0) { That said, I should probably change the order in which I apply the patches locally ... Sorry for the troubles. __ Vlad
From: vlmarek [...] volny.cz
Oh, I sent the patch too soon, sorry. When testing on sparc, I found out that I have to disable few more tests in the 51_* file. I'll rework the patch and attach it again. Another possibility would be to disable all the tests.
On Thu Feb 11 18:26:47 2016, neuron wrote: Show quoted text
> Hi, > > Second issue I have found while packing DBD::SQLite to Solaris. We > ship sqlite without > column metadata feature. That causes two issues > > a) DBD::SQLite can't be compiled, because of the > -DSQLITE_ENABLE_COLUMN_METADATA parameter used during build. > b) tests against this feature are failing. > > The attached patch tries to detect such a condition and act > accordingly. The only way I can think off how to detect the > compilation options in Makefile.PL is to run sqlite binary and ask. It > tries to find the binary in the same way how sqlite.h is located. If > the detection fails, Makefile.PL should behave as before - use > -DSQLITE_ENABLE_COLUMN_METADATA compile option. > > I have tested this with sqlite library on Solaris which does not have > column metadata compiled in. Also I have tested building on Solaris > with the builtin sqlite.c.
Thanks for the patch, but it seems (to me) a bit overengineered. Migth it be enough to see if ($^O eq 'solaris' && !$sqlite_local) and remove DSQLITE_ENABLE_COLUMN_METADATA from @CC_DEFINE if true? As for the test failures because of the lack of ENABLE_COLUMN_METADATA, could you test the latest 1.50 I shipped today? I suppose they have been fixed already.
From: vlmarek [...] volny.cz
Show quoted text
> Thanks for the patch, but it seems (to me) a bit overengineered.
Fair enough. Show quoted text
> Migth it be enough to see if ($^O eq 'solaris' && !$sqlite_local) and > remove DSQLITE_ENABLE_COLUMN_METADATA from @CC_DEFINE if true?
For me, yes, that will work. But I don't think that the problem is Solaris centric. Any system with sqlite compiled without the column metadata option will IMO have the same problem. Another way of detecting the absence of column metadata would be to link sqlite to a test program. I believe some symbols would be missing. Another option is to ignore the problem on the base that it is so minor that it's not worth the trouble. That is also fine with me. Basically I'm happy to move any direction you like :) Show quoted text
> As for the test failures because of the lack of > ENABLE_COLUMN_METADATA, could you test the latest 1.50 I shipped > today? I suppose they have been fixed already.
Will do, thank you -- Vlad
From: vlmarek [...] volny.cz
Show quoted text
> As for the test failures because of the lack of > ENABLE_COLUMN_METADATA, could you test the latest 1.50 I shipped > today? I suppose they have been fixed already.
Yes, I can confirm that. So the only problem is to detect that condition before compilation. Thank you __ Vlad
On Fri Feb 12 01:08:33 2016, neuron wrote: Show quoted text
> > Thanks for the patch, but it seems (to me) a bit overengineered.
> > Fair enough. >
> > Migth it be enough to see if ($^O eq 'solaris' && !$sqlite_local) and > > remove DSQLITE_ENABLE_COLUMN_METADATA from @CC_DEFINE if true?
> > For me, yes, that will work. But I don't think that the problem is > Solaris centric. Any system with sqlite compiled without the column > metadata option will IMO have the same problem.
Theoretically yes, but nobody else has asked us to fix that, probably because nobody else has needed to, or it's more natural for others to remove also -DSQLITE_ENABLE_COLUMN_METADATA when you tweak Makefile.PL (with patch or sed or whatever) to change the if ( 0 ) to if ( 1 ). Generally speaking, a system-specific tweak to avoid some issue on your system should be added in Makefile.PL only if everyone that uses the system is also affected by the issue (like missing pread() in HP-UX and zone malloc issue in older OSX etc). I know this isn't such an issue because I also know from CPAN Testers that metadata itself can be compiled under Solaris. So, I can accept a minimalistic patch for this, but I'd rather like to ask you to just remove the -DSQLITE_ENABLE_COLUMM_METADATA line in your packaging patch, because IMO it's more simple for both of us. Show quoted text
> Another way of detecting > the absence of column metadata would be to link sqlite to a test > program. I believe some symbols would be missing. Another option is to > ignore the problem on the base that it is so minor that it's not worth > the trouble. That is also fine with me. > > Basically I'm happy to move any direction you like :) > >
> > As for the test failures because of the lack of > > ENABLE_COLUMN_METADATA, could you test the latest 1.50 I shipped > > today? I suppose they have been fixed already.
> > Will do, thank you
From: vlmarek [...] volny.cz
Show quoted text
> Theoretically yes, but nobody else has asked us to fix that, probably > because nobody else has needed to, or it's more natural for others to > remove also -DSQLITE_ENABLE_COLUMN_METADATA when you tweak Makefile.PL > (with patch or sed or whatever) to change the if ( 0 ) to if ( 1 ). > > Generally speaking, a system-specific tweak to avoid some issue on > your system should be added in Makefile.PL only if everyone that uses > the system is also affected by the issue (like missing pread() in HP- > UX and zone malloc issue in older OSX etc). I know this isn't such an > issue because I also know from CPAN Testers that metadata itself can > be compiled under Solaris. So, I can accept a minimalistic patch for > this, but I'd rather like to ask you to just remove the > -DSQLITE_ENABLE_COLUMM_METADATA line in your packaging patch, because > IMO it's more simple for both of us.
No problem for me, thanks for looking into this. I'm going to close the bug now. __ Vlad
From: vlmarek [...] volny.cz
Show quoted text
> No problem for me, thanks for looking into this. I'm going to close > the bug now.
Hmmm, I don't see a way to close the bug myself, probably you'll have to do it. Cheers __ Vlad
Closed :)