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

Fix the priority order for environment variables between workspace and context #2476

Closed
wants to merge 2 commits into from
Closed
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
16 changes: 9 additions & 7 deletions doc/usage.rst
Original file line number Diff line number Diff line change
Expand Up @@ -501,13 +501,15 @@ context or can be the description of an opam switch, as follows:
are meant to be executed on the host machine, such as preprocessors.

- ``(paths (<var1> <val1>) .. (<varN> <valN>))`` allows to set the value of any
``PATH``-like variables in this context. If ``PATH`` itself is modified in
this way, its value will be used to resolve binaries in the workspace,
including finding the compiler and related tools. These variables will also be
passed as part of the environment to any program launched by ``dune``. For
each variable, the value is specified using the :ref:`ordered-set-language`.
Relative paths are interpreted with respect to the workspace root, see
:ref:`finding-root`.
``PATH``-like variables in this context. These variables are passed as part of
the environment to any program launched by ``dune``. For each variable, the
value is specified using the :ref:`ordered-set-language`. Relative paths are
interpreted with respect to the context build directory.


If environment variables, such as ``PATH``, are modified in the `dune-workspace`
through `env-vars` or `paths`, its value will be used to resolve binaries in the
workspace, including finding the compiler and related tools.

Both ``(default ...)`` and ``(opam ...)`` accept a ``targets`` field in order to
setup cross compilation. See :ref:`advanced-cross-compilation` for more
Expand Down
27 changes: 19 additions & 8 deletions src/context.ml
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ module Env_nodes = struct

let extra_env ~profile env_nodes =
Env.extend_env
(Dune_env.Stanza.find env_nodes.context ~profile).env_vars
(Dune_env.Stanza.find env_nodes.workspace ~profile).env_vars
(Dune_env.Stanza.find env_nodes.context ~profile).env_vars
end

type t =
Expand Down Expand Up @@ -407,7 +407,6 @@ let create ~(kind : Kind.t) ~path ~env ~env_nodes ~name ~merlin ~targets
Option.value ~default:Env.empty
(Option.map findlib_config ~f:Findlib.Config.env)
)
|> Env.extend_env (Env_nodes.extra_env ~profile env_nodes)
in
let stdlib_dir = Path.of_string (Ocaml_config.standard_library ocfg) in
let natdynlink_supported = Ocaml_config.natdynlink_supported ocfg in
Expand Down Expand Up @@ -531,7 +530,7 @@ let create ~(kind : Kind.t) ~path ~env ~env_nodes ~name ~merlin ~targets
in
native :: List.filter_opt others

let extend_paths t ~env =
let extend_paths t ~env ~build_dir =
let module Eval =
Ordered_set_lang.Make(String)
(struct
Expand All @@ -550,7 +549,13 @@ let extend_paths t ~env =
in
let vars =
let to_absolute_filename s =
Path.of_string s |> Path.to_absolute_filename in
if Filename.is_relative s then
Path.Build.relative build_dir s
|> Path.build
|> Path.to_absolute_filename
else
s
in
let sep = String.make 1 Bin.path_sep in
let env = Env.Map.of_list_exn t in
let f l = String.concat ~sep (List.map ~f:to_absolute_filename l) in
Expand Down Expand Up @@ -646,9 +651,17 @@ let instantiate_context env (workspace : Workspace.t)
; workspace = workspace.env
}
in
let env =
Env.extend_env env
(Env_nodes.extra_env
~profile:(Workspace.Context.profile context) env_nodes)
in
let build_dir =
Path.Build.relative Path.Build.root (Workspace.Context.name context) in
let env = extend_paths ~env ~build_dir (Workspace.Context.paths context) in
match context with
| Default { targets; name; host_context = _; profile; env = _
; toolchain ; paths; loc = _ } ->
; toolchain ; paths = _; loc = _ } ->
let merlin =
workspace.merlin_context = Some (Workspace.Context.name context)
in
Expand All @@ -657,13 +670,11 @@ let instantiate_context env (workspace : Workspace.t)
| Some _ -> toolchain
| None -> Env.get env "OCAMLFIND_TOOLCHAIN"
in
let env = extend_paths ~env paths in
default ~env ~env_nodes ~profile ~targets ~name ~merlin ~host_context
~host_toolchain
| Opam { base = { targets; name; host_context = _; profile; env = _
; toolchain; paths; loc = _ }
; toolchain; paths = _; loc = _ }
; switch; root; merlin } ->
let env = extend_paths ~env paths in
create_for_opam ~root ~env_nodes ~env ~profile ~switch ~name ~merlin
~targets ~host_context ~host_toolchain:toolchain

Expand Down
1 change: 1 addition & 0 deletions src/stdune/env.mli
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ val get : t -> Var.t -> string option
val extend : t -> vars:string Map.t -> t

val extend_env : t -> t -> t
(** [extend_env x y] gives priority to the values of y *)

val add : t -> var:Var.t -> value:string -> t

Expand Down
8 changes: 8 additions & 0 deletions src/workspace.ml
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,14 @@ module Context = struct
| Default x -> x.targets
| Opam x -> x.base.targets

let profile = function
| Default x -> x.profile
| Opam x -> x.base.profile

let paths = function
| Default x -> x.paths
| Opam x -> x.base.paths

let all_names t =
let n = name t in
n :: List.filter_map (targets t) ~f:(function
Expand Down
3 changes: 3 additions & 0 deletions src/workspace.mli
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ module Context : sig

val env : t -> Dune_env.Stanza.t

val profile : t -> string
val paths : t -> (string * Ordered_set_lang.t) list

val host_context : t -> string option
end

Expand Down
2 changes: 1 addition & 1 deletion test/blackbox-tests/test-cases/env-variables/run.t
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ used.
$ dune exec --root precedence ./printenv.exe VARIABLE_FROM_BOTH
Entering directory 'precedence'
Entering directory 'precedence'
VARIABLE_FROM_BOTH=from_workspace
VARIABLE_FROM_BOTH=from_context

When a variable is repeated, an error message is displayed:

Expand Down
2 changes: 1 addition & 1 deletion test/blackbox-tests/test-cases/workspace-paths/bin/dune
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
(executable
(name hello)
(promote (until-clean)))
Copy link
Member

Choose a reason for hiding this comment

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

You need $ make fmt here

)
Binary file not shown.
6 changes: 4 additions & 2 deletions test/blackbox-tests/test-cases/workspace-paths/run.t
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
$ dune build
$ dune build bin/hello.exe

$ dune build @default
Copy link
Member

Choose a reason for hiding this comment

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

why $ dune build @default over just $ dune build?

hello alias default
Hello: $TESTCASE_ROOT/a:/c
Hello: $TESTCASE_ROOT/_build/default/a:/c

$ mkdir sub
$ cat > sub/dune-workspace <<EOF
Expand Down