On Tue, Mar 04, 2008 at 12:21:43PM -0600, Norm Jacobs wrote:
> >If I make no other changes then I don't need to make the above changes
> >to avoid getting the .libs directory in the runpath. It is possible
> >that the changes you suggest + the changes I was using will do what I
> >wanted, but I ended up pursuing a much simpler approach, which you can
> >see at the current webrev:
> >
> >http://cr.opensolaris.org/~nico/webrev-sqlite3-sfw-the-3rd/
> >
> I guess that the biggest issues with this approach is that it is
> relatively foreign to how many of the other SFW components build.
Well, some SFW components already roll their own build/install parts, as
I am doing. I predict that we'll see a lot odd things in the SFW
consolidation.
It would help if FOSS projects had guidelines that they could, and
hopefuly would, follow that would make integration into our SFW
consolidation easier.
> There is something to be said for attempting to achieve some
> uniformity in the gate.
I doubt that's achievable... :(
> I realize that not everything will fit in the
> box, but in cases were it just does (or almost does), it would be
> better if the more uniform approach were taken. However, since you
> are signing up to support this in perpetuity, perhaps this is less of
> an issue ;-) . Though if you are hit by a bus, someone is going to be
> muttering under their breath.
Well, I can add comments, if it'll help. Also, perpetuity >
expected_lifetime(self). So technically I've not signed up to support
this in perpetuity; most likely what team is responsible for SQLite3
upkeep in SFW will vary over time.
> >The SQLite community has a penchant for static linking. For one thing
> >it means that you're not dependent on someone else possibly changing
> >versions of SQLite on you, and for another it means slightly faster
> >code. But we're talking about integrating SQLite for Solaris consumers,
> >so dynamically linking with libsqlite3.so is the right answer here.
> >
> I guess, to me, it seems like more of a flaw in their build system
> that should be rectified. If I would like to build and deliver shared
> libraries, there is no reason that anything in the sqlite code base
> should be statically linking in the objects already delivered in it's
> own shared libraries.
Maybe, but Dr. Hipp likes it this way (he generally recommends
statically linking libsqlite3 into apps). That's fine as general advice
to SQLite3 consumers, but within Solaris / OpenSolaris, that's not
appropriate.
Now, if I can get the SQLite3 community to accept patches that are
intended for use only by the SFW consolidation, then I'll pursue that
approach. I'll ask off-line.
> I tried not to duplicate comments that have already been made.
>
> usr/src/lib/sqlite3/Makefile.sfw:
> lines 71-81:
> You might put one configure option per line and use
> CONFIGURE_OPTIONS += --options
> instead of line continuation.
> You have two --with-tcl options in your CONFIGURE_OPTIONS
> It looks like you may not need the following configure options:
> --exec-prefix defaults to {prefix}
> --bindir defaults to {exec_prefix}/bin
> --libdir defaults to {exec_prefix}/lib
> --includedir defaults to {exec_prefix}/include
> Do you really want to deliver the extra debugging code via
> --enable-debug?
It gets you the ability to trace the SQLite VM. OTOH, there's a comment
in the code about performance going faster if one defines NDEBUG, so I
may want to add -DNDEBUG=1 here.
Alternatively I could omit VM tracing.
Also, I don't need to set any of --prefix and friends because I no
longer gmake install.
> You might set CC, CPPFLAGS, and LDFLAGS in the calling env with the
> rest if of the env variables you are passing to configure.
True. I need to clean that out, no?
> lines 172, 183:
> If there isn't a compelling reason to keep the original
> Makefile.in, I wouldn't
> bother. It can always be extracted from the tarball.
Right. I'll remove this before integration. It's been helpful to me
during development.
> lines 193-200:
> You might consider unpacking and patching the source once and simply
> copying it from $(VER) to $(VER64).
I just did consider it, but I don't see much improvement.
> usr/src/lib/sqlite3/Makefile.in.diff:
> Your patch to their build system is Solaris specific. It would be
> preferable to make a more generic set of fixes that you send
> upstream so that we have a chance of not having to create a new
> patch if/when we resync with the upstream project.
Yes, but see my comments above.
> line 21:
> I would think that you would only need to touch target_source
> once when you are done with your for loop
True, I may do this instead: "touch target_source-" in the loop and "mv
-f target_source- target_source" outside. The point is to only touch it
if anything had to be updated. (Without this patch target_source is
always remade and so solaris/libsqlite3.so.0 is remade for the
executable and libtclsqlite3.so.)
> Enabling the readline support is probably a good thing, but I have two
> "issues" with it. First, your ARC case specifically called out that
> you would be excluding this support. Second, how does this impact the
> licensing / copyright files that you are using in the packages? We
> may need counsel to weigh in and provide some clarity :-\ .
I discovered that readline was available in the gate after the ARC case
approval. My intention was to send a note to the case about this.
However, it turns out that the c-team is mighty uncomfortable with the
sqlite3 use of readline, so I'm going to remove it. I tried
libeditline, and that works with minimal changes to shell.c, BUT, I
can't get history to work -- only line editing works. So I'm going to
remove readline and I'm not going to add editline support.
I'll update the webrev later today, or maybe tomorrow.
