Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update utf8proc, replace wcwidth #10659

Merged
merged 1 commit into from
Mar 30, 2015
Merged

Conversation

stevengj
Copy link
Member

This updates utf8proc to version 1.2 (#10654) and replaces calls to the often-outdated wcwidth with utf8proc_charwidth (#3721 and #6939).

(It seemed simpler to leave it as a submodule and just rename it from libmojibake back to utf8proc.)

Note that USE_SYSTEM_UTF8PROC=1 will no longer work unless the system has utf8proc 1.2 or later (cc @nalimilan).

The patch also uses the new function utf8proc_category to get the category code rather than looking inside the utf8proc_property_t struct (which assumed that the category is the first 16-bit field).

@stevengj stevengj added the unicode Related to unicode characters and encodings label Mar 28, 2015
@stevengj stevengj changed the title update utf8proc update utf8proc, replace wcwidth Mar 28, 2015
@tkelman
Copy link
Contributor

tkelman commented Mar 29, 2015

For AppVeyor, will need to do s/mojibake/utf8proc/g in contrib/windows/msys_build.sh

@stevengj
Copy link
Member Author

Thanks @tkelman, just pushed a fixed commit.

@tkelman
Copy link
Contributor

tkelman commented Mar 29, 2015

Looks like wcwidth is also used in flisp. And the undefined symbols issue with __imp is probably related to the conflicting definitions of DLLEXPORT. Maybe #undef DLLEXPORT after including dtypes.h but before including uft8proc.h.

@stevengj stevengj force-pushed the utf8proc-new branch 7 times, most recently from 95e29d2 to 4813b29 Compare March 30, 2015 14:05
@stevengj
Copy link
Member Author

I don't really understand why the Makefile for flisp used the -DUTF8PROC_EXPORT flag; shouldn't this only be used when compiling utf8proc (so that it adds the dllexport decorator)? Is it because we're going to link utf8proc and flisp into the same library in the end? In that case, I guess we now need -DUTF8PROC_EXPORT when compiling libsupport (since that now calls utf8proc_charwidth).

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2015

We only build the static library of utf8proc, but we link that static library into the shared libjulia. We want the shared libjulia to export these utf8proc symbols. At least I'm pretty sure that's how it works. MSVC is pickier than MinGW in these matters, but I can worry about that later.

@nalimilan
Copy link
Member

Why don't we link dynamically to utf8proc?

@stevengj
Copy link
Member Author

@nalimilan, because it is a dependency of libjulia, not just of Base, utf8proc has to be included in libjulia itself (inter-library dependencies are especially problematic on Windows).

(FYI, if you do have a separate utf8proc library, e.g if USE_SYSTEM_UTF8PROC=1, we require utf8proc 1.2 or later. Fortunately, the portion of the ABI that Julia uses is backwards compatible, so utf8proc 1.2 should also work with older versions of Julia.)

Looks like I just had a typo: was defining UTF8PROC_EXPORT rather than UTF8PROC_EXPORTS

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2015

Libjulia already has dll dependencies, namely libgcc and libstdc++. It's really not so bad on Windows, you just can't have undefined references and the dependencies have to be specified at link time. And the dll dependencies need to be available either in the same folder as the binary or on the path somewhere (I prefer the former).

@nalimilan
Copy link
Member

OK, you know what you're doing. :-)

@stevengj
Copy link
Member Author

(Yes, but this means that on Windows it's quite hard to dlopen a library with dependencies on another library ... the contortions that numpy goes through to be openable dynamically and yet still callable from other Python modules that are loaded dynamically are quite atrocious.) Dependencies on libstdc++ and (to a lesser extent) libgcc are less problematic because those are so commonplace.

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2015

We dlopen (via ccall in JuliaOpt packages) the entire COIN-OR stack which is a big complicated interdependent set of shared libraries just fine. It occasionally requires some patching to make sure the dependencies are linked properly at build time, but that's not so bad. I have no idea what numpy is doing wrong there, but it's not so hard to do right. Numpy is just such a rat's nest that no one wants to touch it (I apparently managed to talk Travis Oliphant out of wanting to interview me for that job by being too honest about how uninteresting it would be to work on fixing Python's Windows problems...).

@stevengj
Copy link
Member Author

Arg, now appveyor is succeeding, but one of the Travis jobs timed out; possibly unrelated?

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2015

Yeah, was on osx so most likely. There have been some efforts to optimize the tests a little recently, but apparently didn't get all the way to reliably finishing in time.

@stevengj
Copy link
Member Author

Argh, one of the appveyor builds is failing now, too. All I did was push a NEWS update; another timeout?

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2015

another timeout?

Yeah, froze at linalg1 only on 64 bit, that's been a long-standing intermittent mystery.

@stevengj
Copy link
Member Author

So, is this mergeable?

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2015

I think so, may as well.

stevengj added a commit that referenced this pull request Mar 30, 2015
update utf8proc, replace wcwidth
@stevengj stevengj merged commit 92538cf into JuliaLang:master Mar 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unicode Related to unicode characters and encodings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants