Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added check on package names in dependencies #9472

Merged
merged 2 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/dune_lang/package_dependency.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 }
;;

Expand Down
57 changes: 20 additions & 37 deletions test/blackbox-tests/test-cases/invalid-package-version.t
Original file line number Diff line number Diff line change
@@ -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 <<EOF
> (lang dune 3.13)
Expand All @@ -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 <<EOF
> (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:

Expand All @@ -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]
19 changes: 13 additions & 6 deletions test/blackbox-tests/test-cases/pkg/invalid-version.t
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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]
Loading