408603 - Windows sqlite3.dll should have proper version information

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Attachment: Add sqlite RC file

Currently, sqlite3.dll is being built without a proper resource file, causing it not have any version information shown with it.

The attached patch provides an RC file and the needed makefile changes for it to work.

One note: I'm not overly happy with the current setup of having to define SQLITE_VERSION_MAJOR, SQLITE_VERSION_MINOR, and SQLITE_VERSION_PATCH within the RC file, but unfortunately sqlite3.h only provides the version as an x.y.z string and as an integer equal to x*1000000 + y*1000 + z, and as much as I tried, I couldn't find a way to break that down in such a way that the resource compiler could handle. I've filed an upstream bug with sqlite (http://www.sqlite.org/cvstrac/tktview?tn=2847) to try to get them to add them to future versions of sqlite3.h. Otherwise, we'll have to continue manually changing the numbers whenever we update sqlite versions on the trunk.

Comment 1

[(not currently active) Ted Mielczarek]

You could write a tiny python script (or probably even just do it with sed or whatever) to generate a header file to include in the .rc file.  Something like:

sqlite-version.h: sqlite3.h
  do-something > sqlite-version.h

Comment 2

[Shawn Wilsher :sdwilsh]

Note (since sqlite's bug tacker sucks and you have to pull to get updates on tickets):  Upstream ticket has new comment that may be useful.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

I responded to the ticket asking for permission to use the script. What I'll probably do is have it output to sqlite-version.h like Ted suggested and include that in the RC file. Seems like a better option than messing around with sqlite3.h.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 293427 [details] [diff] [review]
Add sqlite RC file

r- per discussion.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Attachment: Use separate sqlite-version.h file generated at build time

Well, I learned something today! Here's a new patch which generates the SQLITE_VERSION numbers with an awk script called by the sqlite3 makefile during the build process.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 295509 [details] [diff] [review]
Use separate sqlite-version.h file generated at build time

One big comment, and a nit.

First, I'm just not excited about adding a new awk script to our build process.  I gather from your comments that you pulled this from the sqlite project and modified it?  If you're just taking it wholesale form upstream I could be convinced, but I'd be much happier with a little python script achieving the same effect.  I can't imagine it'd be a big script, since you're just doing a regex match + a few prints.

nit:

Move the GARBAGE define up above the sqlite-version.h rule, and leave a blank line in between the two.

Aside from the whole awk thing, this patch looks good.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Attachment: Use separate sqlite-version.h file generated at build time - now with more Python!

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 296121 [details] [diff] [review]
Use separate sqlite-version.h file generated at build time - now with more Python!

Looks good, just a few nits on your Python script.

>Index: db/sqlite3/src/sqlite-version.py
>===================================================================
>+# Define RegEx for finding and breaking apart SQLITE_VERSION string
>+versionString = "^#define SQLITE_VERSION\D*(\d+)\.(\d+)\.(\d+)(?:\.(\d+))?\D*"

You can use re.compile here to get a compiled regex object, and then just use its .search method down in the loop.

>+#Grab command line argument pointing to sqlite3.h and open the file
>+arg = sys.argv[1]
>+sqlite3h = open(arg).readlines()

I'd just do open(sys.argv[1]) here, also

>+for line in range(len(sqlite3h)):

Instead of grabbing .readlines(), you can just iterate over the file object, which iterates over the lines:

for line in open(sys.argv[1]):

This makes |line| the text of each line.

>+    if result is None:    #If RegEx doesn't match, go on to next loop iteration
>+        continue

I'd probably reverse this test, |if result is not None:|, then pull the next few lines of code and the print statements inside that if, then  you can just |break| out of the for loop at the end, so your script quits when you hit the line you're looking for.


Everything else looks good, r=me with those nits fixed.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Attachment: Address review comments

(In reply to comment #8)
> (From update of attachment 296121 [details] [diff] [review])
> You can use re.compile here to get a compiled regex object, and then just use
> its .search method down in the loop.

Per our post-review conversation on IRC, I'm leaving as-is for style reasons.

> I'd just do open(sys.argv[1]) here, also
> Instead of grabbing .readlines(), you can just iterate over the file object,
> which iterates over the lines:
> 
> for line in open(sys.argv[1]):
> 
> This makes |line| the text of each line.
> 

Done

> >+    if result is None:    #If RegEx doesn't match, go on to next loop iteration
> >+        continue
> 
> I'd probably reverse this test, |if result is not None:|, then pull the next
> few lines of code and the print statements inside that if, then  you can just
> |break| out of the for loop at the end, so your script quits when you hit the
> line you're looking for.
> 

Done

Carrying over previous r+ and requesting a1.9.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 296294 [details] [diff] [review]
Address review comments

a=beltzner for 1.9

Comment 11

[Reed Loden [:reed]]

Checking in db/sqlite3/src/Makefile.in;
/cvsroot/mozilla/db/sqlite3/src/Makefile.in,v  <--  Makefile.in
new revision: 1.33; previous revision: 1.32
done
RCS file: /cvsroot/mozilla/db/sqlite3/src/sqlite-version.py,v
done
Checking in db/sqlite3/src/sqlite-version.py;
/cvsroot/mozilla/db/sqlite3/src/sqlite-version.py,v  <--  sqlite-version.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/db/sqlite3/src/sqlite.rc,v
done
Checking in db/sqlite3/src/sqlite.rc;
/cvsroot/mozilla/db/sqlite3/src/sqlite.rc,v  <--  sqlite.rc
initial revision: 1.1
done