HEADSUP: BSD iconv coming to the base system with default off

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

HEADSUP: BSD iconv coming to the base system with default off

Gabor Kovesdan-6
Hi Folks,

I've got some time again to keep working on iconv. The last time I
posted a patch, there were problems with some ports but apart from this
it proved to be quite mature, so I decided to commit it to the base
system but the default setting will be disabled. It can be enabled by
setting the WITH_ICONV knob but whoever does it will take into account
that it may break some ports from being built. However, this change will
help me with future work and will also help interested people in getting
involved in the testing. The rather big changeset is coming soon.

Regards,
Gabor Kovesdan
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-i18n
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: HEADSUP: BSD iconv coming to the base system with default off

Anonymous-86
Anonymous <[hidden email]> writes in "[CFT] BSDL iconv in base system":

> I guess gettext hanging is due to ABI incompatibility, too.
>
>   $ cat foo.po
>   msgid ""
>   msgstr ""
>   "Content-Type: text/plain; charset=UTF-8\n"
>   "Content-Transfer-Encoding: 8bit\n"
>
>   msgid "don’t"
>   msgstr "do not"
>
>   $ msgmerge foo.po /dev/null # GNU iconv
>   . done.
>   msgid ""
>   msgstr ""
>   "Content-Type: text/plain; charset=UTF-8\n"
>   "Content-Transfer-Encoding: 8bit\n"
>
>   #~ msgid "don’t"
>   #~ msgstr "do not"
>
>   $ msgmerge foo.po /dev/null # BSD iconv
>   . done.
>   msgid ""
>   load: 0.10  cmd: msgmerge 65132 [runnable] 2.74r 2.72u 0.00s 23% 2844k
>   ^C
>   (gdb) bt
>   #0  _citrus_iconv_none_iconv_convert (ci=0x80f00f190, in=0x7fffffff0b40, inbytes=0x7fffffff0b40, out=0x7fffffff0b48, outbytes=0x7fffffff0b50, flags=0,
>       invalids=0x7fffffff0aa0) at /usr/src/lib/libiconv_modules/iconv_none/citrus_iconv_none.c:119
>           len = 1
>           e2big = 0
>   #1  0x00000008064b9142 in _citrus_iconv_convert (cv=0x80f00f190, in=0x7fffffff0b38, inbytes=0x7fffffff0b40, out=0x7fffffff0b48, outbytes=0x7fffffff0b50,
>       flags=0, nresults=0x7fffffff0aa0) at citrus_iconv.h:60
>   No locals.
>   #2  0x00000008064b90b2 in libiconv (handle=0x80f00f190, in=0x7fffffff0b38, szin=0x7fffffff0b40, out=0x7fffffff0b48, szout=0x7fffffff0b50)
>       at /usr/src/lib/libc/iconv/iconv.c:147
>           ret = 0
>           err = 0
>   #3  0x00000008036db20a in wrap (mp=0x80f020400, stream=0x80f0123c0, line_prefix=0x0, extra_indent=0, css_class=0x80370a2f0 "msgstr",
>       name=0x80370a3a9 "msgstr", value=0x80f01f0b0 "Content-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n", do_wrap=undecided,
>       page_width=79, charset=0x7fffffff0d80 "UTF-8") at write-po.c:724
>   #4  0x00000008036dcbdd in message_print (mp=0x80f020400, stream=0x80f0123c0, charset=0x7fffffff0d80 "UTF-8", page_width=79, blank_line=false, debug=false)
>       at write-po.c:1283
>   #5  0x00000008036dd736 in msgdomain_list_print_po (mdlp=0x80f0071c0, stream=0x80f0123c0, page_width=79, debug=false) at write-po.c:1511
>   #6  0x00000008036d8859 in msgdomain_list_print (mdlp=0x80f0071c0, filename=0x80370a0a6 "standard output", output_syntax=0x40d7b0, force=false, debug=false)
>       at write-catalog.c:246
>   #7  0x0000000000403604 in main (argc=3, argv=0x7fffffff0ff0) at msgmerge.c:463

Above test still hangs as of r219023M with similar trace.
Should I file a PR or bsdiconv isn't supposed to work in such a way?

>
> It's a bit tweaked version, though.
>
> %%
> Index: devel/gettext/Makefile
> ===================================================================
> RCS file: /a/.cvsup/ports/devel/gettext/Makefile,v
> retrieving revision 1.87
> diff -u -p -r1.87 Makefile
> --- devel/gettext/Makefile 3 Jun 2010 09:46:38 -0000 1.87
> +++ devel/gettext/Makefile 23 Aug 2010 10:04:26 -0000
> @@ -28,7 +28,7 @@ CONFIGURE_ENV= ACLOCAL="${TRUE}" \
>   AUTOHEADER="${TRUE}" \
>   MAKEINFO="makeinfo --no-split" \
>   CPPFLAGS="-I${LOCALBASE}/include" \
> - LDFLAGS="-L${LOCALBASE}/lib" \
> + LDFLAGS="-L${LOCALBASE}/lib -liconv" \
>   EMACS="no"
>  CONFIGURE_ARGS= --disable-csharp --disable-threads --disable-openmp \
>   --with-included-gettext --with-included-glib \
> @@ -65,6 +65,8 @@ pre-extract:
>  .endif
>  
>  post-patch:
> + @${REINPLACE_CMD} 's/-DENABLE_RELOCATABLE=1//' \
> + ${WRKSRC}/gettext-runtime/intl/Makefile.in
>   @${FIND} ${WRKSRC} -name configure -print | ${XARGS} \
>      ${REINPLACE_CMD} -e 's|mkdir gmkdir|mkdir|'
>  .if defined (NOPORTDOCS)
> %%
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-i18n
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: HEADSUP: BSD iconv coming to the base system with default off

Alexander Kabaev-3
In reply to this post by Gabor Kovesdan-6
On Thu, 24 Feb 2011 22:40:48 +0000
Gabor Kovesdan <[hidden email]> wrote:

> Hi Folks,
>
> I've got some time again to keep working on iconv. The last time I
> posted a patch, there were problems with some ports but apart from
> this it proved to be quite mature, so I decided to commit it to the
> base system but the default setting will be disabled. It can be
> enabled by setting the WITH_ICONV knob but whoever does it will take
> into account that it may break some ports from being built. However,
> this change will help me with future work and will also help
> interested people in getting involved in the testing. The rather big
> changeset is coming soon.
>
> Regards,
> Gabor Kovesdan
Hi Gabor,

for whatever historic reason I had WITH_ICONV in /etc/make.conf on my
desktop, so I got to be your guinea pig without conscious consent for
that role on my part. I did hit several issues so far:

1. mutt-devel port does not consider our implementation as 'good
enough'. It runs the test below, which returns 1, while GNU libiconv
returns 0.

| #include <iconv.h>
| int main()
| {
|   iconv_t cd;
|   char buf[4];
|   char *ob;
|   size_t obl;
|   ob = buf, obl = sizeof(buf);
|   return ((cd = iconv_open("UTF-8", "UTF-8")) != (iconv_t)(-1) &&
|           (iconv(cd, 0, 0, &ob, &obl) ||
|            !(ob == buf && obl == sizeof(buf)) ||
|            iconv_close(cd)));
| }

2. The implementation of internal locking in citrus_locking.h is a
strange one. Here is why:

Do you really want to have the lock declared as static in header file?
Even is so, please note that declaring data in header file is confusing
and having three locks with the same name is not making the library any
easier to debug.
 
You need to use proper XXX_STATIC_INITIALIZER constant to initialize
the lock statically, instead of declaring it as static. The code only
works on FreeBSD due to luck and will break if our typedefs for
pthreads lock primitives will ever grow to be the proper structures
instead of simple pointers.

The locking in citrus_mapper.c is broken in case of parallel and
sequential mappers. The file grabs the lock and calls _mapper_open. If
mapper being loaded happens to be of a composite type, it will in turn
try to invoke _mapper_open on subordinate mappers recursively and will
deadlock waiting for the lock it already owns.

And at last, by are you using rwlock if all locking you ever do is an
exclusive one?

3. There are dangerous vestiges of iconv.dir support in citrus_iconv.c
file, _citrus_iconv_open function. You call _lookup_alias there using
an uninitialized path variable, causing every iconv-using program to
try and access <garbage>.db file. While there, since you are not using
iconv.dir, would it make sense to just merge iconv_std and iconv_none
into libc? Same goes to various flavors of libmapper_*.so, is thee a
reason why each comes in its own trivial shared library?

4. Most of _citrus_<blah> functions are internal interface between
encoders and mappers and do nto constitute public ABI we as a project
are signing on to support forever. As such, they belong in private
namespace and should not pollute FBSD1.2.
 

--
Alexander Kabaev

signature.asc (195 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: HEADSUP: BSD iconv coming to the base system with default off

Gabor Kovesdan-6
Hi Alexander,
> for whatever historic reason I had WITH_ICONV in /etc/make.conf on my
> desktop, so I got to be your guinea pig without conscious consent for
> that role on my part. I did hit several issues so far:
thanks for your valuable comments about iconv and I'm sorry that you had
to try it out without explicit willingness. I didn't count with such
special cases like this one.

Unfortunately, for a short period I've run out again of free time but
I'll work out the solution for the problems you described. I'd like to
comment on only one specific one now, the modular architecture. As you
might know this implementation comes from the Citrus Project and the
original authors designed it in such a way for flexibility and easy
extensibility. It is modular at various levels of abstraction. Maybe
this modularity isn't really required in real life and reducing the
overhead from module handling the performance may improve. But so far I
haven't investigated on such changes because I wanted to concentrate on
the functional part first, but in the future, I'll definitely consider
changes on the modular architecture.

Gabor
_______________________________________________
[hidden email] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-i18n
To unsubscribe, send any mail to "[hidden email]"