William A. Rowe, Jr. wrote:
At 12:36 AM 3/30/2005, Curt Arnold wrote:
However, it would be good if we could get some comment from a Subversion developer.
Agreed, and Branko's watching this thread.
Yup.
It might also be a good thing to ping Steve King of TortoiseSVN fame; TSVN also ships a hacked version of apr-iconv, I believe the hack is to ignore APR_ICONV_PATH. The issue IIRC is collision between memory allocators in different runtime librararies on Windows; but anyway, Steve should be able to explain.
The reasons why TSVN ships a patched version of apr-iconv:
- the *.so files are actually dll's which require a specific C-runtime. This isn't a big problem if APR_ICONV_PATH points to a iconv compiled with VC6. But if it points to an iconv compiled with >VC7, then programs not using the new c-runtime (> version 7) won't run anymore. A 'solution' to that problem would be to install the runtime in e.g. the system dir on windows, but even MS tells to _not_ do that.
- every program which installs its own iconv lib might change the APR_ICONV_PATH variable (believe me: most programs _don't_ check if it's already set) and overwrite this with maybe an older version of the lib!
- looking up the env variable isn't very fast, i.e. it takes more time to load the *.so modules than other methods.
The changes I made to apr-iconv for TSVN:
- add a DllMain() to iconv_module which caches the path to the *.so modules (i.e. the path is determined only once when the iconv dll is loaded, not for every *.so module) - this is a _very_ big speed improvement for Subversion which makes intensive use of *.so modules.
- the path is determined like this now:
(in the following description, THISPATH refers to the path where the libapriconv.dll is located, fetched via GetModuleFileName() API)
1. path exists THISPATH\iconv
2. path exists ..\THISPATH\iconv
3. use APR_ICONV_PATH
by doing this, it's assured that a program like TSVN always uses the iconv lib it has installed itself. Only if it doesn't install the iconv lib itself or it can't be found in 1) or 2), then the APR_ICONV_PATH env variable is used to find the *.so modules.
You can find the patched iconv_module.c file here: https://svn.collab.net/repos/tortoisesvn/trunk/ext/apr-iconv_patch/lib/iconv_module.c
Ever since TSVN ships with this patched iconv version, we haven't had any more problems with other Subversion clients interfering or missing c-runtime dll's or memory leaks due to incompatible c-runtimes (according to MS, the version 6 and 7 of the c-runtime are not compatible and use different memory allocators which leads to memory leaks and even heap corruptions if a program uses the wrong version).
So I'm not sure what you guys have planned to change in apr-iconv, but I strongly recommend that if you change something, get rid of the APR_ICONV_PATH (on windows at least), or at least use it as the last option like TSVN does now.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
