ctypes external_library_name doesn't support a value which doesn't translate to a valid ocaml module name

[Khady]

Expected Behavior

When using external_library_name icu-io dune shouldn't throw an error. It should translate the name into something valid in ocaml instead.

Actual Behavior

Description:
  ("Invalid Module_name.t", { s = "icu-io__c_generated_types" })
Raised at Stdune__code_error.raise in file "otherlibs/stdune/code_error.ml",
  line 11, characters 30-62
Called from Dune_rules__ctypes_rules.Stanza_util.generated_modules in file
  "src/dune_rules/ctypes_rules.ml", line 129, characters 8-39
Called from Dune_rules__ctypes_rules.Stanza_util.generated_ml_and_c_files in
  file "src/dune_rules/ctypes_rules.ml", line 142, characters 6-30
Called from Dune_rules__dir_contents.Load.load_text_files.(fun) in file
  "src/dune_rules/dir_contents.ml", line 175, characters 31-75
Called from Fiber.Scheduler.exec in file "src/fiber/fiber.ml", line 854,
  characters 10-13
-> required by ("dir-contents-get0", ("default", "default/icu"))
-> required by ("gen-rules", "default/icu")
-> required by ("load-dir", In_build_dir "default/icu")
-> required by
   ("build-alias", { dir = "default/icu"; name = "default" })
-> required by ("toplevel", ())

The file icu_io__c_library_flags.sexp also contains ("-licu_io") which is incorrect

$ pkg-config --libs icu-io
-licuio -licui18n -licuuc -licudata

Reproduction

(library
 (name icu)
 (public_name icu)
 (libraries )
 (ctypes
  (external_library_name icu-io)
  (build_flags_resolver pkg_config)
  (headers (include "unicode/ubrk.h" "unicode/utext.h"))
  (type_description
   (instance Type)
   (functor Type_description))
  (function_description
   (concurrency unlocked)
   (instance Function)
   (functor Function_description))
  (generated_types Types_generated)
  (generated_entry_point C)
  ))

Specifications

• Version of dune (output of dune --version): 3.0.3
• Version of ocaml (output of ocamlc --version): 4.12.1
• Operating system (distribution and version): debian 10

[bobot]

cc @mbacarella

[mbacarella]

Hmm, right. It takes a string though it writes out intermediate module names derived from that string.

Should be a pretty simple fix. What's the space of valid module names? Is it [A-Z][a-z0-9_]*?

[Khady]

at least [A-Z][a-zA-Z0-9_]*

[rr0gi]

and apostrophe https://ocaml.org/manual/lex.html#sss:lex:identifiers

[Khady]

is there more to do than fixing Module_name.of_string (or writing an alternative version that correctly mangles the name)?

[bobot]

I would prefer an alternative version which mangles the name. The fix should also check that the name given in the external_library_name field of the stanza can be mangled, otherwise give a nice message, the code is here:

dune/src/dune_rules/ctypes_stanza.ml

Lines 128 to 135 in 8677fe3

	(let+ external_library_name = field "external_library_name" string
	and+ build_flags_resolver =
	field_o "build_flags_resolver" Build_flags_resolver.decode
	and+ type_description = field "type_description" Type_description.decode
	and+ function_description =
	multi_field "function_description" Function_description.decode
	and+ headers = field_o "headers" Headers.decode
	and+ generated_types = field_o "generated_types" Module_name.decode

You could use the StringLike module to easily create a decoder:

dune/src/dune_util/stringlike.mli

Line 6 in 8677fe3

module Make (S : Stringlike_intf.S_base) : Stringlike_intf.S with type t = S.t

like the one Module_name.decode:

dune/src/dune_engine/section.ml

Line 59 in 8677fe3

Stringlike.Make (struct

[Khady]

The fix should also check that the name given in the external_library_name field of the stanza can be mangled, otherwise give a nice message

What would be a name that can't be mangled?

[bobot]

It is perhaps not the mangling that would fail, but perhaps something else. If the whole toolchain can support (external_library_name "foo!#what?/oups~ in space") we can keep the string decoder otherwise it is better to not get another backtrace somewhere, no?

[Khady]

@bobot is it what you were thinking about? https://github.com/ocaml/dune/pull/5667/files

The next step would be to do (external_lib_name |> External_lib_name.to_module_name |> Module_name.to_string) in the right places I believe.

[Khady]

Potentially fixed by #5667

[Khady]

The file icu_io__c_library_flags.sexp also contains ("-licu_io") which is incorrect

I don't know how to test for that

[Khady]

$ make test
./dune.exe runtest
File "test/blackbox-tests/test-cases/ctypes/lib-external-name-need-mangling.t/run.t", line 1, characters 0-0:
------ test/blackbox-tests/test-cases/ctypes/lib-external-name-need-mangling.t/run.t
++++++ test/blackbox-tests/test-cases/ctypes/lib-external-name-need-mangling.t/run.t.corrected
File "test/blackbox-tests/test-cases/ctypes/lib-external-name-need-mangling.t/run.t", line 13, characters 0-1:
 |Bind to a C library which has a name invalid in ocaml
 |
 |  $ LIBEX=$(realpath "$PWD/../libneed-mangling")
 |
 |This silly looking hack is to make sure the .pc file points to the sandbox. We
 |cannot set ${prefix} to be interpreted relative to the .pc itself ufortunately
 |  $ awk "BEGIN{print \"prefix=$LIBEX\"} {print}" $LIBEX/need-mangling.pc > need-mangling.pc
 |
 |  $ DYLD_LIBRARY_PATH="$LIBEX" LD_LIBRARY_PATH="$LIBEX" PKG_CONFIG_PATH="$PWD:$PKG_CONFIG_PATH" dune exec ./example.exe
 |  4
 |
 |  $ cat _build/default/stubgen/need-mangling__c_library_flags.sexp
+|  ("-L/workspace_root/test/blackbox-tests/test-cases/ctypes/libneed-mangling" "-lneed-mangling")

looks ok

[Khady]

Fixed by 3075bd4