Windows sqlite3.dll should have proper version information

RESOLVED FIXED in mozilla1.9beta3

Status

()

Toolkit
Storage
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

Trunk
mozilla1.9beta3
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 293427 [details] [diff] [review]
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.
Attachment #293427 - Flags: review?(comrade693+bmo)
(Assignee)

Updated

11 years ago
Attachment #293427 - Flags: review?(comrade693+bmo) → review?(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
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.
(Assignee)

Comment 3

11 years ago
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 on attachment 293427 [details] [diff] [review]
Add sqlite RC file

r- per discussion.
Attachment #293427 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 5

11 years ago
Created attachment 295509 [details] [diff] [review]
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.
Attachment #293427 - Attachment is obsolete: true
Attachment #295509 - Flags: review?(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.
Attachment #295509 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 296121 [details] [diff] [review]
Use separate sqlite-version.h file generated at build time - now with more Python!
Attachment #295509 - Attachment is obsolete: true
Attachment #296121 - Flags: review?(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.
Attachment #296121 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 9

11 years ago
Created attachment 296294 [details] [diff] [review]
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.
Attachment #296121 - Attachment is obsolete: true
Attachment #296294 - Flags: review+
Attachment #296294 - Flags: approval1.9?
Comment on attachment 296294 [details] [diff] [review]
Address review comments

a=beltzner for 1.9
Attachment #296294 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.