fix: handle utf8 characters in the dune files(#9728) by moyodiallo · Pull Request #10113 · ocaml/dune

moyodiallo · 2024-02-22T13:02:49Z

This PR is about principally handling utf8 characters in quoted strings.

rgrinberg · 2024-02-22T18:26:13Z

Why do we need to copy paste code from the stdlib?

moyodiallo · 2024-02-22T19:15:35Z

Why do we need to copy paste code from the stdlib?

No need, we could write our own functions. But I did this because I used functions introduced in OCaml.4.14 (later version).

rgrinberg · 2024-02-22T19:45:54Z

The issue isn’t the copy pasting of some code itself, it’s just that I think we’re copying too much. Don’t we already have a subset of Uchar present since our minimum version is 4.08? Could we just add only the code we need?

…

On Feb 22, 2024, at 11:15 AM, Alpha Issiaga DIALLO ***@***.***> wrote: Why do we need to copy paste code from the stdlib? No need, we could write our own functions. But I did this because I used functions introduced in OCaml.4.14 (later version). — Reply to this email directly, view it on GitHub <#10113 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABB56Z5TEQ43NYGCIMYW3DYU6KOJAVCNFSM6AAAAABDU5EZVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGA4TEMBVGE>. You are receiving this because you commented.

moyodiallo · 2024-02-22T19:56:54Z

I've just tested uutf, I'm going to use vendor/uutf present in Dune project. That is sufficient for this PR.

moyodiallo · 2024-02-22T19:59:25Z

Indeed uutf use Uchar which is present in OCaml 4.08.

moyodiallo · 2024-02-24T12:32:12Z

The copy paste is removed and uutf is used instead.

rgrinberg · 2024-03-10T15:39:42Z

It's a bit sad, but this PR has a non trivial increase in pressure on the GC. Since this fix only affects a tiny fraction of users, is it possible to implement this in a less intrusive way?

moyodiallo · 2024-03-11T09:41:13Z

Yes it is possible. I'll try to fix it.

emillon · 2024-03-11T16:32:15Z

I don't think that the lexer changes are useful for this change. Just changing Escape will fix the formatter, which was the original issue. The parsing error for %{...} will continue refering to individual bytes, but I think that's fine compared to changing the lexer.

src/dune_sexp/escape.ml

moyodiallo · 2024-03-11T16:42:36Z

I don't think that the lexer changes are useful for this change. Just changing Escape will fix the formatter, which was the original issue. The parsing error for %{...} will continue refering to individual bytes, but I think that's fine compared to changing the lexer.

I get it but it won't be coherent, accepting utf8 char and printing escaped characters.

moyodiallo · 2024-03-15T09:40:58Z

It's a bit sad, but this PR has a non trivial increase in pressure on the GC. Since this fix only affects a tiny fraction of users, is it possible to implement this in a less intrusive way?

It's fixed.

emillon

this will also require a changelog entry

src/dune_sexp/escape.mli

src/dune_sexp/lexer.mll

test/blackbox-tests/test-cases/formatting/non-ascii-characters.t

moyodiallo · 2024-03-15T17:13:28Z

this will also require a changelog entry

The changelog entry added.

src/dune_sexp/escape.ml

emillon · 2024-03-19T10:41:33Z

Thanks. I'll adapt the API so that it's more in line with the stdlib API and thus easier to remove later.

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com> Signed-off-by: Etienne Millon <me@emillon.org>

Also leaving a note about using the stdlib later. Signed-off-by: Etienne Millon <me@emillon.org>

Signed-off-by: Etienne Millon <me@emillon.org>

emillon · 2024-03-19T12:12:41Z

I tried to use the stdlib way, but doing so and adding polyfills for 4.08 meant that stdune would depend on uutf. So instead I adapted the code to use uutf decoder (that work on 4.08) in dune_sexp with a note on how to migrate to stdlib when we bump the compat.

emillon · 2024-03-19T12:13:46Z

@rgrinberg is the library structure (dune_sexp depending on dune_uutf) sensible?

rgrinberg

Seems fine to me. Make sure to double check the benchmarks before merging.

emillon · 2024-03-20T09:35:58Z

Benchmarks look good so I'm merging this, thanks.

@frejsoya

CHANGES: ### Added - Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya) - Remove some unnecessary limitations in the expansions of percent forms in install stanza. For example, the `%{env:..}` form can be used to select files to be installed. (ocaml/dune#10160, @rgrinberg) - Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in more contexts. Previously, they would be randomly forbidden in some fields. (ocaml/dune#10169, @rgrinberg) - Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg) - Remove limitations on percent forms in the `(enabled_if ..)` field of libraries (ocaml/dune#10250, @rgrinberg) - Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon) - Allow defining executables or melange emit stanzas with the same name in the same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri) ### Fixed - coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego) - Fix conditional source selection with `select` on `bigarray` in OCaml 5 (ocaml/dune#10011, @moyodiallo) - melange: fix inconsistency in virtual library implementation. Concrete modules within a virtual library can now refer to its virtual modules too (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro) - melange: fix a bug that would cause stale `import` paths to be emitted when moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190, @anmonteiro) - Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113, fixes ocaml/dune#9728, @moyodiallo) - Fix expanding dependencies and locks specified in the cram stanza. Previously, they would be installed in the context of the cram test, rather than the cram stanza itself (ocaml/dune#10165, @rgrinberg) - Fix bug with `dune exec --watch` where the working directory would always be set to the project root rather than the directory where the command was run (ocaml/dune#10262, @gridbugs) - Regression fix: sign executables that are promoted into the source tree (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon) - Fix crash when decoding dune-package for libraries with `(include_subdirs qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon) ### Changed - Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)

@frejsoya

CHANGES: ### Added - Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya) - Remove some unnecessary limitations in the expansions of percent forms in install stanza. For example, the `%{env:..}` form can be used to select files to be installed. (ocaml/dune#10160, @rgrinberg) - Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in more contexts. Previously, they would be randomly forbidden in some fields. (ocaml/dune#10169, @rgrinberg) - Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg) - Remove limitations on percent forms in the `(enabled_if ..)` field of libraries (ocaml/dune#10250, @rgrinberg) - Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon) - Allow defining executables or melange emit stanzas with the same name in the same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri) ### Fixed - coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego) - Fix conditional source selection with `select` on `bigarray` in OCaml 5 (ocaml/dune#10011, @moyodiallo) - melange: fix inconsistency in virtual library implementation. Concrete modules within a virtual library can now refer to its virtual modules too (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro) - melange: fix a bug that would cause stale `import` paths to be emitted when moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190, @anmonteiro) - Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113, fixes ocaml/dune#9728, @moyodiallo) - Fix expanding dependencies and locks specified in the cram stanza. Previously, they would be installed in the context of the cram test, rather than the cram stanza itself (ocaml/dune#10165, @rgrinberg) - Fix bug with `dune exec --watch` where the working directory would always be set to the project root rather than the directory where the command was run (ocaml/dune#10262, @gridbugs) - Regression fix: sign executables that are promoted into the source tree (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon) - Fix crash when decoding dune-package for libraries with `(include_subdirs qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon) ### Changed - Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)

moyodiallo force-pushed the allow-utf8-characters branch from 920470b to 4f73860 Compare February 22, 2024 13:04

moyodiallo force-pushed the allow-utf8-characters branch from 4f73860 to 0907f5f Compare February 24, 2024 12:27

moyodiallo force-pushed the allow-utf8-characters branch from 0907f5f to 2b45467 Compare February 24, 2024 12:33

emillon reviewed Mar 11, 2024

View reviewed changes

src/dune_sexp/escape.ml Outdated Show resolved Hide resolved

moyodiallo force-pushed the allow-utf8-characters branch 4 times, most recently from f255718 to 509b6f5 Compare March 15, 2024 09:12

moyodiallo requested a review from emillon March 15, 2024 09:41

emillon reviewed Mar 15, 2024

View reviewed changes

src/dune_sexp/escape.mli Outdated Show resolved Hide resolved

src/dune_sexp/lexer.mll Outdated Show resolved Hide resolved

test/blackbox-tests/test-cases/formatting/non-ascii-characters.t Show resolved Hide resolved

moyodiallo force-pushed the allow-utf8-characters branch from 509b6f5 to 1d34180 Compare March 15, 2024 17:11

moyodiallo requested a review from emillon March 15, 2024 17:12

rgrinberg requested changes Mar 19, 2024

View reviewed changes

src/dune_sexp/escape.ml Outdated Show resolved Hide resolved

moyodiallo force-pushed the allow-utf8-characters branch from 1d34180 to b52cef4 Compare March 19, 2024 10:46

moyodiallo requested a review from rgrinberg March 19, 2024 10:47

moyodiallo force-pushed the allow-utf8-characters branch from b52cef4 to 8cb27fc Compare March 19, 2024 10:49

fix: handle utf8 characters in the dune files(ocaml#9728)

bf65d22

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com> Signed-off-by: Etienne Millon <me@emillon.org>

moyodiallo and others added 3 commits March 19, 2024 13:10

Add the changes file and revert the modification of the lexer

8f9c9c8

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com> Signed-off-by: Etienne Millon <me@emillon.org>

Use uutf

bafb443

Also leaving a note about using the stdlib later. Signed-off-by: Etienne Millon <me@emillon.org>

changelog

f413a42

Signed-off-by: Etienne Millon <me@emillon.org>

emillon force-pushed the allow-utf8-characters branch from 8cb27fc to f413a42 Compare March 19, 2024 12:10

rgrinberg approved these changes Mar 19, 2024

View reviewed changes

Merge branch 'main' into allow-utf8-characters

221ffec

emillon merged commit 171c231 into ocaml:main Mar 20, 2024

moyodiallo deleted the allow-utf8-characters branch March 20, 2024 09:46

moyodiallo restored the allow-utf8-characters branch March 20, 2024 09:46

moyodiallo deleted the allow-utf8-characters branch March 20, 2024 09:47

rgrinberg added this to the 3.15.0 milestone Mar 20, 2024

Leonidas-from-XIV mentioned this pull request Mar 26, 2024

[new release] dune (15 packages) (3.15.0~alpha1) ocaml/opam-repository#25585

Closed

anmonteiro mentioned this pull request Mar 27, 2024

Synopsis & description characters are escaped #9728

Closed

Leonidas-from-XIV mentioned this pull request Apr 3, 2024

[new release] dune (15 packages) (3.15.0) ocaml/opam-repository#25615

Merged

moyodiallo mentioned this pull request May 3, 2024

Fix lint errors ocsigen/js_of_ocaml#1607

Closed

Conversation

moyodiallo commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgrinberg commented Feb 22, 2024

Uh oh!

moyodiallo commented Feb 22, 2024

Uh oh!

rgrinberg commented Feb 22, 2024 via email

Uh oh!

moyodiallo commented Feb 22, 2024

Uh oh!

moyodiallo commented Feb 22, 2024

Uh oh!

moyodiallo commented Feb 24, 2024

Uh oh!

rgrinberg commented Mar 10, 2024

Uh oh!

moyodiallo commented Mar 11, 2024

Uh oh!

emillon commented Mar 11, 2024

Uh oh!

Uh oh!

moyodiallo commented Mar 11, 2024

Uh oh!

moyodiallo commented Mar 15, 2024

Uh oh!

emillon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moyodiallo commented Mar 15, 2024

Uh oh!

Uh oh!

emillon commented Mar 19, 2024

Uh oh!

emillon commented Mar 19, 2024

Uh oh!

emillon commented Mar 19, 2024

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

emillon commented Mar 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moyodiallo commented Feb 22, 2024 •

edited

Loading