dune init --public: allow dashes etc

[reynir]

An attempt to fix #7108

[reynir]

This seems to allow too much. I can't quite figure out what functions to use and what exactly is allowed in a public_name.

[rgrinberg]

Anything accepted by Lib_name.of_string should be allowed no?

[reynir]

Hmm, I guess so. The failing existing test just made me doubt this solution.

[rgrinberg]

Could you add a DCO?

[reynir]

I added a signed-off-by line. Let me know if I did it right. It's not cryptographically signed - my PGP key expired and I lost the password to the master key /o\

[Alizter]

@reynir We don't need GPG signing only the Signed-off message in the commit.

[Alizter]

You probably just want User_error.raise here.

[Alizter]

The tests are failing since it looks like names are not being validated correctly:

  $ dune init lib foo --public="some/invalid&name!"
-  dune: option '--public': invalid component name `some/invalid&name!'
-        Library names must be non-empty and composed only of the
-        following
-        characters: 'A'..'Z', 'a'..'z', '_' or '0'..'9'.
-  Usage: dune init library [OPTION]… NAME [PATH]
-  Try 'dune init library --help' or 'dune --help' for more information.
-  [1]
+  Success: initialized library component named foo

[reynir]

Thank you for your review @Alizter.

Regarding the failing test I am aware. In the code base I wasn't able to find any function for validating public names (very possible I missed something) nor what is allowed in a public name. In other words, it's unclear to me if the failing test is actually invalid and if so how to check it. @rgrinberg suggests "[a]nything accepted by Lib_name.of_string should be allowed", and this is what it checks. Is this a bug with Lib_name.of_string then?

As for your comments about the code: the new code is basically the same as component_name_conv just using Lib_name.of_string_opt instead of Lib_name.Local.of_string_opt. In component_name_conv there's already a comment saying (* TODO refactor to use Lib_name.Local.conv *) -- maybe both should be refactored then? I would appreciate help with this as I am very unfamiliar with the dune codebase.

[reynir]

As for the failing test, I tried to run dune exec -- dune init lib foo --public="some/invalid&name!" with this patch and then build it:

dune/tmp$ dune build
Entering directory '/home/reynir/workspace/dune'
File "tmp/dune", line 2, characters 14-32:
2 |  (public_name some/invalid&name!)
                  ^^^^^^^^^^^^^^^^^^
Error: The current scope doesn't define package "some/invalid&name!".
The only packages for which you can declare elements to be installed in this
directory are:
- chrome-trace       (because of chrome-trace.opam)
- dune               (because of dune.opam)
- dune-action-plugin (because of dune-action-plugin.opam)
- dune-build-info    (because of dune-build-info.opam)
- dune-configurator  (because of dune-configurator.opam)
- dune-glob          (because of dune-glob.opam)
- dune-private-libs  (because of dune-private-libs.opam)
- dune-rpc           (because of dune-rpc.opam)
- dune-rpc-lwt       (because of dune-rpc-lwt.opam)
- dune-site          (because of dune-site.opam)
- dyn                (because of dyn.opam)
- ocamlc-loc         (because of ocamlc-loc.opam)
- ordering           (because of ordering.opam)
- stdune             (because of stdune.opam)
- xdg                (because of xdg.opam)

I then edited dune-project and added the following section:

(package
 (name some/invalid&name!)
 (synopsis "test")
 (depends
   (ocaml (>= 4.08.0)))
 (description "Just testing an invalid package name"))

After this it seemed to successfully build. But if I cd'ed to the project root I got the following error when building (long time since I saw that error message!):

$ dune build
Internal error, please report upstream including the contents of _build/log.
Description:
  ("invalid alias name", { s = ".some/invalid&name!-files" })
Raised at Stdune__code_error.raise in file
  "otherlibs/stdune/src/code_error.ml", line 11, characters 30-62
Called from Dune_rules__dep_conf_eval.package_install in file
  "src/dune_rules/dep_conf_eval.ml", line 15, characters 2-77
Called from Dune_rules__install_rules.gen_package_install_file_rules in file
  "src/dune_rules/install_rules.ml", line 1026, characters 23-74
Called from Fiber__scheduler.exec in file "otherlibs/fiber/src/scheduler.ml",
  line 73, characters 8-11
-> required by ("<unnamed>", ())
-> required by ("load-dir", In_build_dir "default")
-> required by
   ("build-alias", { dir = In_build_dir "default"; name = "default" })
-> required by ("toplevel", ())

I must not crash.  Uncertainty is the mind-killer. Exceptions are the
little-death that brings total obliteration.  I will fully express my cases. 
Execution will pass over me and through me.  And when it has gone past, I
will unwind the stack along its path.  Where the cases are handled there will
be nothing.  Only I will remain.

[Alizter]

I think it is fine not to validate public_name as it is technically correct in the error message. We should however be validating the package stanza in dune-project files.

[rgrinberg]

@Alizter can you push this PR along?

[Alizter]

@rgrinberg I would like to expose a public name validation API in Lib_name, analogous to Lib_name and Lib_name.Local. There is a location in the code today but the validation is essentially left as a todo.

I don't have time to look into that at the moment, but @reynir if you are interested you can the relevant code in Lib_name.

[reynir]

@Alizter I don't really have the time, and the code base is large and unfamiliar to me. I also have no clue what is a valid public name and what is not.

[Alizter]

@reynir No worries, I'll continue this when I have some time.

[rgrinberg]

@emillon i think you've fixed this?

[emillon]

Yes, this has been done in #8603.