From 9a2eb33888af109bc7727af6e3c0af10c0c19405 Mon Sep 17 00:00:00 2001 From: Ambre Austen Suhamy Date: Wed, 20 Dec 2023 12:02:22 +0100 Subject: [PATCH] Added check on package names in dependencies Signed-off-by: Ambre Austen Suhamy --- src/dune_lang/package_dependency.ml | 30 +++++++++- .../test-cases/invalid-package-version.t | 57 +++++++------------ .../test-cases/pkg/invalid-version.t | 19 +++++-- 3 files changed, 62 insertions(+), 44 deletions(-) diff --git a/src/dune_lang/package_dependency.ml b/src/dune_lang/package_dependency.ml index 1839408eba0..98fdff08988 100644 --- a/src/dune_lang/package_dependency.ml +++ b/src/dune_lang/package_dependency.ml @@ -5,6 +5,34 @@ type t = ; constraint_ : Package_constraint.t option } +module Well_formed_name = struct + module T = struct + include Package_name.Opam_compatible + + let module_ = "Package.Dependency.Name" + let description = "package dependency" + let description_of_valid_string = Some description_of_valid_string + + let better_hint s = + match String.lsplit2 s ~on:'.' with + | None -> make_valid s + | Some (before, after) -> + if Option.is_some (of_string_opt before) && not (String.is_empty after) + then sprintf "(%s (= %s))" before after + else make_valid s + ;; + + let hint_valid = Some better_hint + end + + module TT = Dune_util.Stringlike.Make (T) + + let decode = + let open Dune_sexp.Decoder in + TT.decode >>| T.to_package_name + ;; +end + let encode { name; constraint_ } = let open Dune_sexp.Encoder in match constraint_ with @@ -20,7 +48,7 @@ let decode = { name; constraint_ = Some expr } in enter constrained - <|> let+ name = Package_name.decode in + <|> let+ name = Well_formed_name.decode in { name; constraint_ = None } ;; diff --git a/test/blackbox-tests/test-cases/invalid-package-version.t b/test/blackbox-tests/test-cases/invalid-package-version.t index 20485c549b0..49ec8ed05af 100644 --- a/test/blackbox-tests/test-cases/invalid-package-version.t +++ b/test/blackbox-tests/test-cases/invalid-package-version.t @@ -1,6 +1,6 @@ -Invalid package versions should be reported by Dune. We should also give a hint for common -invalid versions such as foo.1.2.3, which can result from a users false assumption from -opam. +Invalid package names are reported by Dune. +We also give a hint for common invalid versions such as foo.1.2.3, +which can result from a users false assumption from opam. $ cat > dune-project < (lang dune 3.13) @@ -10,17 +10,17 @@ opam. > (depends foo.1.2.3)) > EOF -Building doesn't raise any error about the invalid version. +Building doesn't work as the dependency is invalid. $ dune build - -Even generating .opam files doesn't raise any error about the version. - - $ cat >> dune-project < (generate_opam_files true) - > EOF - - $ dune build invalid.opam + File "dune-project", line 5, characters 10-19: + 5 | (depends foo.1.2.3)) + ^^^^^^^^^ + Error: "foo.1.2.3" is an invalid package dependency. + Package names can contain letters, numbers, '-', '_' and '+', and need to + contain at least a letter. + Hint: (foo (= 1.2.3)) would be a correct package dependency + [1] We can take this further and add some really invalid characters into the name: @@ -34,28 +34,11 @@ We can take this further and add some really invalid characters into the name: > EOF $ dune build invalid.opam - - $ cat invalid.opam - # This file is generated by dune, edit dune-project instead - opam-version: "2.0" - depends: [ - "dune" {>= "3.13"} - "f{oo." - "1.2.3" - "odoc" {with-doc} - ] - build: [ - ["dune" "subst"] {dev} - [ - "dune" - "build" - "-p" - name - "-j" - jobs - "@install" - "@runtest" {with-test} - "@doc" {with-doc} - ] - ] - + File "dune-project", line 5, characters 10-15: + 5 | (depends f{oo."1.2.3")) + ^^^^^ + Error: "f{oo." is an invalid package dependency. + Package names can contain letters, numbers, '-', '_' and '+', and need to + contain at least a letter. + Hint: f_oo_ would be a correct package dependency + [1] diff --git a/test/blackbox-tests/test-cases/pkg/invalid-version.t b/test/blackbox-tests/test-cases/pkg/invalid-version.t index b814f81f5ad..38a81f62c6a 100644 --- a/test/blackbox-tests/test-cases/pkg/invalid-version.t +++ b/test/blackbox-tests/test-cases/pkg/invalid-version.t @@ -1,7 +1,7 @@ -Having an invalid package dependency should give a good user message rather than raising -an uncaught exception. It is very likely that users will type foo.1.2.3 for a package -version due to the convention in opam. In this case we could also give a hint how to write -it in a dune-project file. +Having an invalid package dependency that looks like an opam-versioned package +gives a good user message rather. It is very likely that users will type +foo.1.2.3 for a package version due to the convention in opam. +In this case we could also hint at the correct syntax for dune-project files. $ . ./helpers.sh $ mkrepo @@ -21,5 +21,12 @@ it in a dune-project file. > (source "file://$(pwd)/mock-opam-repository")) > EOF - $ dune pkg lock 2>&1 | head -n1 - Error: exception Failure("Invalid character in package name \"foo.1.2.3\"") + $ dune pkg lock 2>&1 + File "dune-project", line 4, characters 10-19: + 4 | (depends foo.1.2.3)) + ^^^^^^^^^ + Error: "foo.1.2.3" is an invalid package dependency. + Package names can contain letters, numbers, '-', '_' and '+', and need to + contain at least a letter. + Hint: (foo (= 1.2.3)) would be a correct package dependency + [1]