Hi Ingo,
Thanks a lot for review. I addressed you comments in a newer patch
http://lists.mysql.com/commits/70285
> Since I'm not a Windows expert, I'm unsure if I'm the right person to
> do
> a review here. To be safe, just replace me with another reviewer. But I
> looked at it anyway and have some comments.
I asked Windows experts too. And I'm sure you're a right person as
a) good reviewer
b) your work (sometimes?, also? , even?) on Windows
c) you know what this WL is about and could you it to proceed with federated
so I think review from you is in order:)
> I think it is acceptable for now to make the decision .dll <-> .so im
> mysql-test-run for the example engine only. But sooner or later we will
> need a general solution. When this patch is pushed, I'll have to
> implement WL#4338 (Make federated truly pluggable or disable federated
> in 5.1), though in 6.0, I guess.
Yeah, that .dll <-> .so looks hacky. But it was the simplest solution I came
up with.
It seems to me that dynamic plugin support within MTR is weak at the moment
and it is not that simple to test plugins. I believe it would be great if we
could start mtr in a mode where all plugins are automagically loaded. So I
could start mtr --suite=falcon --load-all-plugins and not care how Falcon is
build.
A the moment I need a rather complicated mysql-test-run
--mysqld=--plugin_load=<long_string>
--mysqld=--plugin_dir=<another_long_string> to test falcon as plugin.
If I wanted to test both Falcon and Innodb as DLLs, I would need yet another
tricks to copy ha_falcon.dll and ha_innodb.dll into the same directory and
set --plugin_dir to that directory.
> My standard build process includes a long list of
> WITH_*_STORAGE_ENGINE.
> If the list includes FALCON, I get errors:
> > c:\cygwin\home\istruewing\bzrroot\mysql-6.0-wl3653-
> 1\storage\falcon\transformlib\Transform.h(27) : fatal error C1083:
> Cannot open include file: 'Engine.h': No such file or directory
>
> > c:\cygwin\home\istruewing\bzrroot\mysql-6.0-wl3653-
> 1\storage\falcon\Synchronize.h(27) : fatal error C1083: Cannot open
> include file: 'Mutex.h': No such file or directory
Good you noticed this. I think you also have WITH_EMBEDDED_LIBRARY set, at
least I can reproduce it only with WITH_EMBEDDED_LIBRARY.
It has to do with procedure how embedded is built , it would not work with
relative include path that are in storage/falcon/CMakeLists.txt
I fixed it by changing relative include paths to absolute.
> If I leave out FALCON and FEDERATED, these are successfully built as
> DLLs.
This is as designed. If you leave out some engine, it will
a) be built as DLL, if plug.in indicates support for it
b) will not be compiled in into embedded
> For more comments, please see below.
>
> Vladislav Vaintroub, 22.03.2009 18:59:
> ...
>
> > 2685 Vladislav Vaintroub 2009-03-22
> > WL#3653: Loadable Engine on Windows
>
>
> Personally, I'm a friend of some commentary here. ;-)
Point taken:) Wrote some commentary.
> Neither in the worklog, nor in the patch it is sufficiently explained
> (for me), what happens if the old configure syntax
> WITH_FEDERATED_STORAGE_ENGINE is used. Will the engine become static
> (it
> seems so), or does it depend on its definition like with the
> --with-plugin-federated syntax?
WITH_FEDERATED_STORAGE_ENGINE is handled the same as --with-plugin-federated
(or --with-plugins=federated).Which means static link with mysqld, exactly
as before the patch. I would not like to force bteam or MySQL development to
change their existing scripts ;)
> ...
> > + IF(EXISTS ${SUBDIR}/plug.in)
> > + FILE(READ ${SUBDIR}/plug.in PLUGIN_FILE_CONTENT)
> > + STRING (REGEX MATCH "MYSQL_PLUGIN_DYNAMIC"
> MYSQL_PLUGIN_DYNAMIC ${PLUGIN_FILE_CONTENT})
> > + STRING (REGEX MATCH "MYSQL_PLUGIN_MANDATORY"
> MYSQL_PLUGIN_MANDATORY ${PLUGIN_FILE_CONTENT})
> > + STRING (REGEX MATCH "MYSQL_PLUGIN_STATIC"
> MYSQL_PLUGIN_STATIC ${PLUGIN_FILE_CONTENT})
> > +
> > + IF(MYSQL_PLUGIN_MANDATORY)
> > + SET(WITH_${ENGINE}_STORAGE_ENGINE TRUE)
> > + ENDIF(MYSQL_PLUGIN_MANDATORY)
> > +
> > + IF (WITH_${ENGINE}_STORAGE_ENGINE AND MYSQL_PLUGIN_STATIC)
> > + SET(ENGINE_BUILD_TYPE "STATIC")
> > + ELSEIF(NOT WITHOUT_${ENGINE}_STORAGE_ENGINE AND
> MYSQL_PLUGIN_DYNAMIC)
> > + SET(ENGINE_BUILD_TYPE "DYNAMIC")
> > + ELSE(MYSQL_PLUGIN_MANDATORY)
> > + SET(ENGINE_BUILD_TYPE "NONE")
> > + ENDIF(WITH_${ENGINE}_STORAGE_ENGINE AND MYSQL_PLUGIN_STATIC)
>
>
> I'm not familiar with the cmake syntax. I just guess that this line
> ends
> the IF, which has three sections. But I'm confused by the intermediate
> ELSE(MYSQL_PLUGIN_MANDATORY). If the language would be somewhat
> intuitively to understand, I'd guess that this line belongs to the
> former IF(MYSQL_PLUGIN_MANDATORY). But that one is ended with ENDIF
> already. If you want to have a new condition here, you'd probably use
> ELSEIF instead of ELSE. I guess, you tested the script and it turned
> out
> to be syntactically correct. So what's the magic here?
I think you're correct. CMake interpreter seems to be lax and allowed some
undocumented construct.
Documented is something like
IF (condition1)
ELSEIF (condition2)
ELSEIF (condition3)
ELSE (condition1)
ENDIF (condition1)
So both ELSE and ENDIF should have original condition in braces. CMake 2.6
allows also nicer relaxed form with () after ELSE and ENDIF like in
IF (condition1)
ELSEIF(condition2)
ELSE()
ENDIF()
I do not know why I have not used relaxed ELSE/ENDIF form here... Guess
mainly for uniformity , because older complicated form is everywhere else.
> ...
> > --- a/win/create_def_file.js 2008-05-08 10:11:42 +0000
> > +++ b/win/create_def_file.js 2009-03-22 17:59:37 +0000
> > @@ -0,0 +1,203 @@
> > +// create_exports_file.js
>
>
> This looks inconsistent with the file name.
Right. Fixed.
> > +//
> > +// Copyright (C) 2009 Sun Microsystems
> > +//
> > +// This program is free software; you can redistribute it and/or
> modify
> > +// it under the terms of the GNU General Public License as published
> by
> > +// the Free Software Foundation; version 2 of the License.
> > +//
> > +// This program is distributed in the hope that it will be useful,
> > +// but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +// GNU General Public License for more details.
> > +//
> > +// You should have received a copy of the GNU General Public License
> > +// along with this program; if not, write to the Free Software
> > +// Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-
> 1301 USA
> > +
>
>
> I would highly appreciate a comment that explains in a few sentences,
> what the functionality in this file is about.
Yep, wrote come commentary.
> > +ForReading = 1;
> > +ForWriting = 2;
> > +ForAppending = 8;
> ...
> > +// Extract global symbol names and type from objects
> > +function CollectSymbols()
> > +{
> > + var uniqueSymbols = new Array();
> > +
> > + try
> > + {
> > + /*
> > + Compiler tools use VS_UNICODE_OUTPUT env. variable as
> indicator
> > + that they run within IDE, so they can communicate with IDE
> via
> > + pipes instead of usual stdout/stderr. Refer to
> > + http://blogs.msdn.com/freik/archive/2006/04/05/569025.aspx
> > + for more info.
> > + Unset this environment variable.
> > + */
> > + shell.Environment("PROCESS").Remove("VS_UNICODE_OUTPUT");
> > + }
> > + catch(e){}
> > +
> > + var rspfilename = "dumpsymbols.rsp";
> > + var responseFile = CreateResponseFile(rspfilename);
>
>
> Hmm. Either the jscript interpreter has limited capabilities, or it's
> me. :( I can't see a returned value in the definition of
> CreateResponseFile() (see below). Shouldn't the interpreter detect
> this?
> Or is the last used value in a function implicitly returned? Anyway,
> you
> don't seem to use responseFile in this scope. So the assignment might
> be
> dispensable.
Thanks for catching it. responseFile was a leftover of something I tried
prior and here it is obsolete. I removed the assignment.
The interpreter is ok in this case. It is just so Javascript is a language,
where function is a constructor for an object. Code like
function foo()
{
this.x = 0;
this.y = 1;
}
var v = foo();
Would create an object v of type foo with properties v.x and v.y
initialized to 0 and 1.
In my case with responseFile, it looks I created an object without
properties or methods. If I tried to reference something in responseFile,
JScript would complain "object has no property or method named ...."
