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

Already on GitHub? Sign in to your account

iconv_drv will fail on R15B #284

Closed
helllamer opened this Issue Jan 19, 2012 · 13 comments

Comments

Projects
None yet
4 participants
Contributor

helllamer commented Jan 19, 2012

erl_driver API changed: http://groups.google.com/group/erlang-programming/browse_thread/thread/9bcd23ed628ef5e6

Possible iconv fixes here: https://github.com/mihawk/erlang-iconv but gcc fails due to useless/wrong function declaration at iconv_drv.c:89

Owner

arjan commented Jan 20, 2012

Thanks for reporting. We're aware of this issue and are currently discussing a solution vector, @mmzeeman has written his own iconv NIF so we might switch to that one. https://github.com/mmzeeman/eiconv

Owner

mworrell commented May 4, 2012

We should check if we can start using Maas' iconv driver.

@ghost ghost assigned mmzeeman May 4, 2012

Contributor

helllamer commented May 4, 2012

It is usable, works ok. I've tested it in production (not in zotonic, other project) with much webpage conversions from (win1251|iso9...|koi8-r -> utf8).

You only need to fix require_otp_vsn from rebar.config (for R15B or higher).

Owner

mworrell commented May 4, 2012

I see that iconv is used in (our fork of) gen_smtp.

We can modify our gen_smtp fork to use eiconv.
I will send Andrew an e-mail to check if he is willing to move to eiconv.

Owner

mmzeeman commented May 5, 2012

It is usable, works ok. I've tested it in production (not in zotonic,
other project) with much webpage conversions from (win1251|iso9...|koi8-r
-> utf8).

You only need to fix require_otp_vsn from rebar.config (for R15B or
higher).

Hi, I use eiconv for the same reason. I added R15.

Maas

Owner

mmzeeman commented May 10, 2012

Need to make it possible to use eiconv as a drop in replacement for iconv. Otherwise I have to make changes to gen_smpt which makes it incompatible with the upstream repo.

Owner

mworrell commented May 10, 2012

Maybe we can add some small iconv.erl module somewhere?

Don't we run into rebar problems with gen_smtp and its dependency on iconv?

Owner

mmzeeman commented May 10, 2012

Yes, simple wrapper module. That may be nice to include with eiconv as well so others can use it as a drop in replacement too.

And no, no rebar problem as there is no explicit dependency for iconv from gen_smtp. We just have some version from somewhere in the deps dir. Funny, looking at the gen_smtp code I see it can already cope with multiple versions of iconv.

Owner

mmzeeman commented May 17, 2012

Iconv is currently used by gen_smtp. To be able to work gen_smtp expects a registered process named iconv. If it can't find it it calls iconv:start/0 and later calls iconv:open(). This means that gen_smtp has quite some knowledge of the internals of iconv.

So I'm hesitating between two options:

  1. Nicely... meaning change gen_smtp so it doesn't know about the internals of iconv and just uses a module iconv to convert. Starting and stopping is arranged elsewere via application:start and a configuration file.

  2. Hack, hack... mimic the way the iconv works in eiconv so gen_smtp doesn't have to change.

Think 1 is the best option. Especially if we can get the changes into the upstream gen_smtp repo. What do you think?

Owner

arjan commented Oct 13, 2012

I'd go for option #1. I created a ticket for this, Vagabond/gen_smtp#29

However I also see that gen_smtp uses a function iconv:conv_chunked which is not implemented in eiconv..?

Owner

mmzeeman commented Oct 14, 2012

It looks like the iconv implementation zotonic is using also does not implement iconv:conv_chunked. Then the alternative in the catch block is used.

Owner

mmzeeman commented Oct 14, 2012

It is called like this:

decode_body(Type, Body, InEncoding, OutEncoding) ->
    NewBody = decode_body(Type, Body),
    CD = case iconv:open(OutEncoding, fix_encoding(InEncoding)) of
        {ok, Res} -> Res;
        {error, einval} -> throw({bad_charset, fix_encoding(InEncoding)})
    end,
    {ok, Result} = try iconv:conv_chunked(CD, NewBody) of
        {ok, _} = Res2 -> Res2
    catch
        _:_ ->
            iconv:conv(CD, NewBody)
    end,
    iconv:close(CD),
    Result.
Owner

mworrell commented Feb 16, 2015

Closing, gen_smtp dependency has been removed, clearing the way to use any icons library with the correct API

@mworrell mworrell closed this Feb 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment