494826 - Compile SQLite with SQLITE_DEBUG defined

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

This will point out when we are breaking assumptions that SQLite makes in debug builds.  Certainly do not want this done in release builds though.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

I pushed this to the try server, but this does seem to do what I'd expect locally on my mac (in terms of preprocessing it).

Comment 2

[Shawn Wilsher :sdwilsh]

Something is wrong with this patch, but I'm not sure what:
make[5]: *** No rule to make target `../../../dist/bin/sqlite.def', needed by `sqlite3.dll'.  Stop.
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_gecko] Error 2
make[2]: *** [tier_gecko] Error 2
make[1]: Leaving directory `/e/builds/sendchange-slave/sendchange-win32-hg/build/objdir'
make[1]: *** [default] Error 2
make: *** [build] Error 2

Comment 3

[Shawn Wilsher :sdwilsh]

I strongly suspect that libs:: is to late for the def file preprocessing.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

This one actually passes the try server.

Comment 5

[Shawn Wilsher :sdwilsh]

Not sure if I need to add sqlite.def to GARBAGE now either.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 379925 [details] [diff] [review]
v1.1

+DEFFILE = $(FINAL_TARGET)/sqlite.def

You don't want FINAL_TARGET, that's something like $(DIST)/bin:
http://mxr.mozilla.org/mozilla-central/source/config/config.mk#88

I'd use $(CURDIR), which is the directory you're in in the objdir.

+export::
+	@$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
+	  $(srcdir)/sqlite.def > $(FINAL_TARGET)/sqlite.def
+

The way you have this written will cause this to be executed every time this directory gets built. I would instead do something like:

$(LIBRARY): $(CURDIR)/sqlite.def

$(CURDIR)/sqlite.def: $(srcdir)/sqlite.def:
  ...

This way make will run your rule only if sqlite.def doesn't exist in the objdir, or if the copy in the srcdir is newer. (In which case it will relink the library as well.)

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Per comments - I pushed this to the try server to make sure it works.

Comment 8

[Shawn Wilsher :sdwilsh]

This failed on try server - the file didn't get copied over in the object directory:
LINK : fatal error LNK1104: cannot open file '/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def'

Not really sure why :/

Comment 9

[Shawn Wilsher :sdwilsh]

Got the same error when changing $(LIBRARY) to exports::

Still need to figure out what to do here.

Comment 10

[(not currently active) Ted Mielczarek]

Can you paste more of the log, specifically the linker commandline that's failing? From that error message, it looks like an MSYS path is making its way into link.exe, which clearly doesn't know what to do with it.

Comment 11

[Shawn Wilsher :sdwilsh]

Sure thing:
link -NOLOGO -DLL -OUT:sqlite3.dll -PDB:sqlite3.pdb -SUBSYSTEM:WINDOWS  sqlite3.obj    sqlite.res -NXCOMPAT -SAFESEH -DYNAMICBASE  -MANIFEST:NO -LIBPATH:"e:/builds/slave/sendchange-win32-hg/build/objdir/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -DEFAULTLIB:mozcrt19 -DEBUG -OPT:REF -OPT:nowin98 -DEF:/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def    kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib   
LINK : fatal error LNK1104: cannot open file '/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def'

Comment 12

[(not currently active) Ted Mielczarek]

(In reply to comment #11)
> -DEF:/e/builds/slave/sendchange-win32-hg/build/objdir/db/sqlite3/src/sqlite.def

That's the problem. MSYS doesn't do the path translation there because there's no space before that path. :-/ You can try changing $(CURDIR)/sqlite3.def to ./sqlite3.def, and see if that fixes it.

Comment 13

[Shawn Wilsher :sdwilsh]

Just pushed the change suggested in comment 12 to the try server.  Let's hope it works.

Comment 14

[Shawn Wilsher :sdwilsh]

Sadly, no luck.  I'm going to try no ./ now.

link -NOLOGO -DLL -OUT:sqlite3.dll -PDB:sqlite3.pdb -SUBSYSTEM:WINDOWS  sqlite3.obj    sqlite.res -NXCOMPAT -SAFESEH -DYNAMICBASE  -MANIFEST:NO -LIBPATH:"e:/builds/slave/sendchange-win32-hg/build/objdir/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -DEFAULTLIB:mozcrt19 -DEBUG -OPT:REF -OPT:nowin98 -DEF:./sqlite.def    kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib   
LINK : fatal error LNK1104: cannot open file './sqlite.def'

Comment 15

[(not currently active) Ted Mielczarek]

Hm, that seems broken.

Comment 16

[Shawn Wilsher :sdwilsh]

that did not work either - any ideas ted?

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Different approach.  Sadly, it means we add a file to the srcdir, but I think it's OK (we already actually do this).

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 387012 [details] [diff] [review]
v1.3

This one passes the try server!  Yay!

Comment 19

[Benjamin Smedberg]

Comment on attachment 387012 [details] [diff] [review]
v1.3

>diff --git a/db/sqlite3/src/Makefile.in b/db/sqlite3/src/Makefile.in

>+# We have to preprocess our def file because we need different symbols in debug
>+# builds exposed that are not built in non-debug builds.
>+$(DEFFILE_NAME): sqlite.def
>+	@$(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
>+	  $(srcdir)/sqlite.def > $(srcdir)/$(DEFFILE_NAME)

Writing a file to the srcdir during a build is not acceptable. Not only might this cause conflicts for people using multiple objdirs, but if you have a readonly srcdir, such as when building from a readonly mount, this operation will fail completely. The file need to be in the objdir.

And, use $@ for the target and $< for the source file instead of including the full name in the rule again.

Comment 20

[Shawn Wilsher :sdwilsh]

I guess I'll need to figure out how to get a win_curdir variable since msys doesn't do the path filtering otherwise.

Comment 21

[Shawn Wilsher :sdwilsh]

I've filed http://www.sqlite.org/cvstrac/tktview?tn=3983 in order to use __dllspec(import) and __dllspec(export) on SQLITE_API, so we don't have to worry about the def file anymore.  I'm not sure how much this is going to hurt OS2 though...

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

un-tested new approach

The try server doesn't like this, but it looks like infra issues as opposed to my patch.  Will work on this more tomorrow.

Comment 23

[Peter Weilbacher]

Shawn, thanks for the heads-up. GCC on OS/2 does support those decorations, too, and they are used in other parts of Mozilla code. So I hope that is not going to hurt us at all, on the contrary. I will give it a try.

Comment 24

[Shawn Wilsher :sdwilsh]

Note that I ended up not going that way with the latest patch because SQLite was making it a bit difficult.

Comment 25

[Shawn Wilsher :sdwilsh]

Attachment: v3.0

This one seemed to work well locally.  Pushed to try server to see how it goes there.

Comment 26

[Shawn Wilsher :sdwilsh]

Comment on attachment 389768 [details] [diff] [review]
v3.0

ted won't be able to get to this until next week at the earliest, so if bsmedberg can get to this first, that'd be great (this bug blocks other bugs that end up blocking bug 455555).

Comment 27

[Peter Weilbacher]

Comment on attachment 389768 [details] [diff] [review]
v3.0

Could you not directly use DEFFILE = $(CURDIR)/sqlite-processed.def?

For this patch not to break OS/2 you have to change $(srcdir)/sqlite.def in ADD_TO_DEF_FILE to $(CURDIR)/sqlite-processed.def, too. I hope that the pre-processing is run on OS/2 before that, still have to try that.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> (From update of attachment 389768 [details] [diff] [review])
> Could you not directly use DEFFILE = $(CURDIR)/sqlite-processed.def?
Are you complaining about the use of $(DEFFILE_NAME)?  I'm using that because I didn't want to copy and paste the name in multiple places (and in fact, I had a typo in one place in one of my earlier attempts).

> For this patch not to break OS/2 you have to change $(srcdir)/sqlite.def in
> ADD_TO_DEF_FILE to $(CURDIR)/sqlite-processed.def, too. I hope that the
> pre-processing is run on OS/2 before that, still have to try that.
I can make that change before I land.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 389768 [details] [diff] [review]
v3.0

You also want to add $(DEFFILE) to GARBAGE. Thanks for sticking through this and getting it done right!

Comment 30

[Peter Weilbacher]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 389768 [details] [diff] [review] [details])
> > Could you not directly use DEFFILE = $(CURDIR)/sqlite-processed.def?
> Are you complaining about the use of $(DEFFILE_NAME)?  I'm using that because I
> didn't want to copy and paste the name in multiple places

OK, but you are always using $(CURDIR)/$(DEFFILE_NAME) together, so you could as well use $(DEFFILE) instead (and then you don't need DEFFILE_NAME at all). Not a big deal, though.

> > For this patch not to break OS/2 you have to change $(srcdir)/sqlite.def in
> > ADD_TO_DEF_FILE to $(CURDIR)/sqlite-processed.def, too. I hope that the
> > pre-processing is run on OS/2 before that, still have to try that.
> I can make that change before I land.

OK, I was wrong before, of course the pre-processor is never run on OS/2 because it's inside the WIN block (and because our .def file is DEF_FILE). What seems to work is to replace the whole ADD_TO_DEF_FILE line with

ADD_TO_DEF_FILE = $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) \
        $(srcdir)/sqlite.def | sed -e '1,/^EXPORTS$$/ d' -e 's,sqlite3,_\0,' \
        -e 's,\ DATA.*$$,,' >> $(DEF_FILE)

so that the preprocessor directly writes to the correct output file. I don't like the duplication this introduces but I don't see a better way.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: v3.1

Addresses review comments.  I've pushed this to the try server to make sure it builds before I land this on mozilla-central.

Comment 32

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/projects/places//rev/918e07973653

Comment 33

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/48a8978d67f6

Comment 34

[Rain [:rain1] (inactive)]

Hm, this seems to have broken my minefield debug build...

link -NOLOGO -DLL -OUT:sqlite3.dll -PDB:sqlite3.pdb -SUBSYSTEM:WINDOWS  sqlite3.obj    sqlite.res -MANIFESTUAC:NO -NXCOMPAT -SAFESEH -DYNAMICBASE  -DEBUG -DEBUGTYPE:CV       -DEF:D:/mozilla-build/msys/comm-centralminefield-debug/db/sqlite3/src/sqlite-processed.def   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib

errors out with

LINK : fatal error LNK1104: cannot open file 'D:/mozilla-build/msys/comm-centralminefield-debug/db/sqlite3/src/sqlite-processed.def'

There should be a / between |comm-central| and |minefield-debug|, but there isn't, which causes things to break. The file sqlite-processed.def does exist, though, in D:\mozilla-build\msys\comm-central\minefield-debug\db\sqlite3\src.

My mozconfig is:
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../../minefield-debug/
ac_add_options --enable-application=browser
ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --with-windows-version=601

and I'm building from within /comm-central/src/.mozilla-trunk, hence the double .. -- I've tried other paths, including absolute ones, and it doesn't seem to make a difference.

Comment 35

[Peter Weilbacher]

Shawn, seeing the thread about missing symbols in mozilla.dev.builds I now wonder why you chose to use #ifdef SQLITE_DEBUG instead of #ifdef DEBUG in sqlite.def? Is that already defined when the preprocessor is called? (I don't have Windows so I didn't test.)

sid0: you might also want to post to mozilla.dev.builds with this kind of problems. You should check the paths in your autoconf.mk to see which one is wrong. (To me this looks like your make version is broken that it cannot correctly resolve the CURDIR variable...)

Comment 36

[Rain [:rain1] (inactive)]

(In reply to comment #35)
> sid0: you might also want to post to mozilla.dev.builds with this kind of
> problems. You should check the paths in your autoconf.mk to see which one is
> wrong. (To me this looks like your make version is broken that it cannot
> correctly resolve the CURDIR variable...)

Bug 506576 was filed and fixed ;)

Comment 37

[Shawn Wilsher :sdwilsh]

It might not actually be defined...

If it is a problem, someone should file a new bug on it