weltling / parle

Parser and lexer for PHP
Other
82 stars 9 forks source link

Add enable-parle-utf32 configureoption #45

Closed mlocati closed 1 year ago

mlocati commented 1 year ago

And fix a couple of errors in package.xml

Close #31

mlocati commented 1 year ago

Here's a sample script (executed in a php:8.2-cli-alpine docker image), where I manually wrote no and yes after the Enable internal UTF-32 support in parle [no] question:

$ apk add $PHPIZE_DEPS libstdc++
$ cd "$(mktemp -d)"
$ curl -sSL https://github.com/mlocati/parle/archive/refs/heads/enable-utf32-package.xml.tar.gz \
    | tar xz --strip-components=1
$ pecl install ./package.xml
[...omissis...]
Enable internal UTF-32 support in parle [no] : no
[...omissis...]
$ docker-php-ext-enable parle
$ php --ri parle

parle

Lexing and parsing support => enabled
Parle version => 0.8.4-dev
Parle internal UTF-32 => no
$ rm "$(php-config --ini-dir)/docker-php-ext-parle.ini"
$ pecl uninstall parle
$ pecl install ./package.xml
[...omissis...]
Enable internal UTF-32 support in parle [no] : yes
[...omissis...]
$ php --ri parle

parle

Lexing and parsing support => enabled
Parle version => 0.8.4-dev
Parle internal UTF-32 => yes
weltling commented 1 year ago

@mlocati thanks for taking this and for the additional fixes. It seems that this should work with like echo yes | pecl install .... I haven't otherwise seen args to pecl to handle this non interactively. Perhaps not that critical for now, anyway.

A question on the side - what is it today regarding the perf? I think for most users the byte based interface would be the one useful, so probably no reason to flip the default. But maybe i'm wrong.

Thanks

mlocati commented 1 year ago

It seems that this should work with like echo yes | pecl install ...

Yep, that's the approach I use in my install-php-extensions script when using pecl. pickle also supports "named" configuration options (you create a file containing --enable-parle-utf32=yes and pass the name of that file name to pickle with --with-configure-options).

what is it today regarding the perf? I think for most users the byte based interface would be the one useful, so probably no reason to flip the default. But maybe i'm wrong.

I absolutely don't know: I simply added support for parle to install-php-extensions, saw that option in the config.m4 file and wondered why it wasn't possible to configure it with pecl.

And that's all the experience I have with parle :wink:.

mlocati commented 1 year ago

PS: in order to check for errors in the package.xml file we can also add a new GitHub Action (like I recently suggested for the redis PHP extension - see https://github.com/phpredis/phpredis/pull/2347/commits/52a261c1d9f90e1e5be439da1e71ca7945b41717)

weltling commented 1 year ago

Having a basic package.xml test would make sense. Say pecl package, pecl install, test, etc. Some simple tests unfortunately didn't work on the xml file itself. Say in the current form, xmllint would still exit with zero. It would seem also, an empty user tag will break the upload on pecl anyway. But agreed anyway, a basic package and install sure would make sense as there might be some library header missing or similar. It should be sure accepted once you have time to file it when you've time.

Thanks

mlocati commented 1 year ago

an empty user tag will break the upload on pecl anyway

It seems that the xsd requires that the <user> xml element must be defined. So, what should we do in this case?

weltling commented 1 year ago

Thanks for following up. I guess there's nothing to do about it. Say, even if the handle is invalid, it cannot be validated without uploading to the channel.

Thanks

mlocati commented 1 year ago

But agreed anyway, a basic package and install sure would make sense as there might be some library header missing or similar.

And here it is :wink: