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

Remove wrong validation for switch names #3265

Merged
merged 4 commits into from
Mar 16, 2020
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
8 changes: 4 additions & 4 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
2.5.0 (unreleased)
Unreleased
------------------

- Add a `--release` option meaning the same as `-p` but without the
package filtering. This is useful for custom `dune` invocation in opam
files where we don't want `-p` (#3260, @diml)

2.4.1 (unreleased)
------------------

- Fix a bug introduced in 2.4.0 causing `.bc` programs to be built
with `-custom` by default (#3269, fixes #3262, @diml)

- Allow contexts to be defined with local switches in workspace files (#3265,
fix #3264, @rgrinberg)

2.4.0 (06/03/2020)
------------------

Expand Down
9 changes: 3 additions & 6 deletions src/dune/context.ml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module Kind = struct
module Opam = struct
type t =
{ root : string option
; switch : Context_name.t
; switch : string
}
end

Expand All @@ -18,10 +18,7 @@ module Kind = struct
| Default -> Dyn.Encoder.string "default"
| Opam o ->
Dyn.Encoder.(
record
[ ("root", option string o.root)
; ("switch", Context_name.to_dyn o.switch)
])
record [ ("root", option string o.root); ("switch", string o.switch) ])
end

module Env_nodes = struct
Expand Down Expand Up @@ -643,7 +640,7 @@ let create_for_opam ~root ~env ~env_nodes ~targets ~profile ~switch ~name
; ( match root with
| None -> []
| Some root -> [ "--root"; root ] )
; [ "--switch"; Context_name.to_string switch; "--sexp" ]
; [ "--switch"; switch; "--sexp" ]
; ( if version < (2, 0, 0) then
[]
else
Expand Down
2 changes: 1 addition & 1 deletion src/dune/context.mli
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ module Kind : sig
module Opam : sig
type t =
{ root : string option
; switch : Context_name.t
; switch : string
}
end

Expand Down
34 changes: 22 additions & 12 deletions src/dune/workspace.ml
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ module Context = struct
| _, _ -> false

let t =
map string ~f:(function
| "native" -> Native
| s -> Named (Context_name.parse_string_exn (Loc.none, s)))
let+ context_name = Context_name.decode in
match Context_name.to_string context_name with
| "native" -> Native
| _ -> Named context_name

let add ts x =
match x with
Expand Down Expand Up @@ -158,7 +159,7 @@ module Context = struct
module Opam = struct
type t =
{ base : Common.t
; switch : Context_name.t
; switch : string
; root : string option
; merlin : bool
}
Expand All @@ -167,29 +168,38 @@ module Context = struct
let open Dyn.Encoder in
record
[ ("base", Common.to_dyn base)
; ("switch", Context_name.to_dyn switch)
; ("switch", string switch)
; ("root", option string root)
; ("merlin", bool merlin)
]

let equal { base; switch; root; merlin } t =
Common.equal base t.base
&& Context_name.equal switch t.switch
&& String.equal switch t.switch
&& Option.equal String.equal root t.root
&& Bool.equal merlin t.merlin

let t ~profile ~x =
let+ switch = field "switch" Context_name.decode
let+ loc_switch, switch = field "switch" (located string)
and+ name = field_o "name" Context_name.decode
and+ root = field_o "root" string
and+ merlin = field_b "merlin"
and+ base = Common.t ~profile in
let default =
(* TODO this needs proper error handling with locations *)
let name = Context_name.to_string switch ^ Common.fdo_suffix base in
Context_name.parse_string_exn (Loc.none, name)
let name =
match name with
| Some s -> s
| None -> (
let name = switch ^ Common.fdo_suffix base in
match Context_name.of_string_opt name with
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not specific to this PR, but I remember that once we discussed the naming of these functions and agreed on:

  • of_string : string -> t for places where we are just converting between data structures and don't expect errors
  • parse_string : string -> (t, ...) result for places where we are parsing data coming from the outside world (here the ... could just be unit)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we have. Here's how interface looks like:

  val of_string : string -> t

  val parse_string_exn : Loc.t * string -> t

So we all need to do is to remove the _exn suffix from parsing.

| Some s -> s
| None ->
User_error.raise ~loc:loc_switch
[ Pp.textf "Generated context name %S is invalid" name
; Pp.text
"Please specify a context name manually with the (name ..) \
field"
] )
in
let name = Option.value ~default name in
let base = { base with targets = Target.add base.targets x; name } in
{ base; switch; root; merlin }
end
Expand Down
4 changes: 3 additions & 1 deletion src/dune/workspace.mli
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ module Context : sig
module Opam : sig
type t =
{ base : Common.t
; switch : Context_name.t
(** Either a switch name or a path to a local switch. This argument
is left opaque as we leave to opam to interpret it. *)
; switch : string
; root : string option
; merlin : bool
}
Expand Down
6 changes: 4 additions & 2 deletions src/stdune/ordering.ml
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,13 @@ let is_eq = function
let min f x y =
match f x y with
| Eq
| Lt -> x
| Lt ->
x
| Gt -> y

let max f x y =
match f x y with
| Eq
| Gt -> x
| Gt ->
x
| Lt -> y