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

Mkdir_p.exec: attempted to create unmanaged dir in Dune 1.9 #2158

Closed
seantalts opened this issue May 14, 2019 · 17 comments · Fixed by #2159 or ocaml/opam-repository#14239
Closed

Mkdir_p.exec: attempted to create unmanaged dir in Dune 1.9 #2158

seantalts opened this issue May 14, 2019 · 17 comments · Fixed by #2159 or ocaml/opam-repository#14239

Comments

@seantalts
Copy link

seantalts commented May 14, 2019

This used to work fine in Dune 1.8.2 at least. I'm asking to run some programs from outside directories, but that shouldn't be a problem right? I'm passing in that outside directory here as the cmdstan env variable (I should probably capitalize it). Here's the dune file: https://github.com/stan-dev/stanc3/blob/e1f2c7359eca43c478cfba07bcdb7dab64c622e4/test/integration/good/code-gen/dune#L45

halair ~/scm/stanc3 (jenkins/end-to-end) $ cmdstan=~/scm/clean/cmdstan dune runtest test/integration/good/code-gen/
Internal error, please report upstream including the contents of _build/log.
Description:
("Mkdir_p.exec: attempted to create unmanaged dir"
 (p (External /Users/xitrium/scm/clean/cmdstan)))
Backtrace:
Raised at file "src/stdune/exn.ml", line 40, characters 5-10
Called from file "set.ml", line 343, characters 35-38
Called from file "set.ml", line 343, characters 25-33
Called from file "src/stdune/set.ml" (inlined), line 36, characters 18-27
Called from file "src/build_system.ml", line 1451, characters 8-60
Called from file "src/fiber/fiber.ml", line 176, characters 6-11
Re-raised at file "src/stdune/exn.ml" (inlined), line 65, characters 38-65
Called from file "src/stdune/exn_with_backtrace.ml", line 17, characters 2-40
Called from file "src/fiber/fiber.ml", line 82, characters 8-15
Re-raised at file "src/stdune/exn.ml" (inlined), line 65, characters 38-65
Called from file "src/stdune/exn_with_backtrace.ml", line 17, characters 2-40
Called from file "src/fiber/fiber.ml", line 82, characters 8-15
Re-raised at file "src/stdune/exn.ml" (inlined), line 65, characters 38-65
Called from file "src/stdune/exn_with_backtrace.ml", line 17, characters 2-40
Called from file "src/fiber/fiber.ml", line 82, characters 8-15
Re-raised at file "src/stdune/exn.ml" (inlined), line 65, characters 38-65
Called from file "src/stdune/exn_with_backtrace.ml", line 17, characters 2-40
Called from file "src/fiber/fiber.ml", line 82, characters 8-15
Re-raised at file "src/stdune/exn.ml" (inlined), line 65, characters 38-65
Called from file "src/stdune/exn_with_backtrace.ml", line 17, characters 2-40
Called from file "src/fiber/fiber.ml", line 82, characters 8-15
                       
I must not segfault.  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.
@rgrinberg
Copy link
Member

Does the directory in question already exists? If so, then this is indeed a regression.

@seantalts
Copy link
Author

Yep, it exists.

@rgrinberg
Copy link
Member

@diml @aalekseyev Actually, I'm wondering if it perhaps it makes sense to forbid this use case. This might be a regression, but it seems a bit off to allow users to run things outside the build dir using the build rules.

@aalekseyev
Copy link
Collaborator

If the action has no targets in that directory, that seems harmless to me, but I don't really understand what's going on here. (we chdir to an external directory, run "make" and that somehow creates targets for us? that seems weird)

@rgrinberg
Copy link
Member

In the example, we run the command and collect its stdout to create a target, so yes it's a bit weird:

(rule
 (targets eight_schools_ncp_summary.expected)
 (action
   (progn
    (run %{dep:stan2_eight_schools_ncp} random seed=1 sample data file=%{dep:eight_schools.data.R} output file=stan2_eight_schools_ncp.csv)
    (with-stdout-to %{targets}
      (run %{env:cmdstan=cmdstan}/bin/stansummary stan2_eight_schools_ncp.csv))
    (with-stdout-to %{targets}
      (run python %{dep:chop_summary.py} %{targets}))
    )))

@aalekseyev
Copy link
Collaborator

Oh, running a command in external directory for its stdout seems totally fine to me.
I thought the chdirs in the other rules were the problem. Is that not so?

@rgrinberg
Copy link
Member

Okay, I think I understand the issue better. I think the issue is caused when we sandbox an external path:

let sandbox_managed_paths ~sandbox_dir t =
  match t with
  | External _ -> t
  | In_source_tree p -> append_local sandbox_dir (Local.append local_src   p)
  | In_build_dir   p -> append_local sandbox_dir (Local.append local_build p)

We're clearly not sandboxing it correctly and that creates a Chdir to an external dir, which in turn makes Mkdir_p.exec throw up.

@rgrinberg
Copy link
Member

@diml why don't we sandbox external paths? Seems like we can just append the external path after the sandbox dir.

@aalekseyev
Copy link
Collaborator

@rgrinberg, I'm not totally familiar with dune sandboxing, but absolute paths are fundamentally harder to change: you must actually rewrite all their occurrences, unlike relative paths where it's enough to "chdir".

Rewriting paths can work for paths that appear on the command-line, but not for the ones that are hardcoded in the script or binary we're running (or get picked up by reading files).

Why are you saying sandboxing is a problem here? I'm still not following.

@ghost
Copy link

ghost commented May 15, 2019

Currently, the only thing we sandbox is the building of the alias module with OCaml 4.02. Nothing else is sandboxed

@ghost
Copy link

ghost commented May 15, 2019

BTW, I believe this is the culprit:

(rule
  (targets eight_schools_ncp)
  (deps eight_schools_ncp.hpp)
  (action
  (chdir %{env:cmdstan=cmdstan}
    (run make -j4 %{targets}))))

@rgrinberg
Copy link
Member

Why are you saying sandboxing is a problem here? I'm still not following.

It is not the problem. I was looking at the wrong rule and did not notice the explicit chdir.

So in this case I agree with Jeremie that we should just not allow to chdir outside the build directory.

@ghost
Copy link

ghost commented May 15, 2019

@seantalts, I believe this stanza will produce a command like this:

$ make -j4 <absolute-path-to-you-dune-project>/test/integration/good/code-gen/eight_schools_ncp

You can verify this by running:

$ dune rule eight_schools_ncp

Or, to see it in makefile syntax:

$ dune rule -m eight_schools_ncp

Was that the intent?

@seantalts
Copy link
Author

seantalts commented May 15, 2019 via email

@ghost
Copy link

ghost commented May 15, 2019

I guess we never considered such a use case. Normally dune only operates inside the workspace. If you build things outside of the workspace, then dune won't track everything that's happening and that might lead to surprising behaviours. Although, unless you run dune instances in parallel, it shouldn't be too bad.

A more idiomatic way to do that with dune would be to copy (or even just symlink) the cmdstan directory into the workspace and use the foreign build sandboxing method.

@seantalts
Copy link
Author

seantalts commented May 15, 2019

Ah, that looks potentially really useful. We're doing a super weird thing here in that this stanc3 compiler emits C++ code that we then want to build and link using cmdstan makefiles into something we then continue testing with dune. Though obviously we could leave the dune ecosystem at any point during this end to end test; I just liked the output diffing part and having it all in one system.

@rgrinberg
Copy link
Member

Hmm, I think I closed this issue previously by accident. In fact, it was fixed in master just now. Thanks for the report @seantalts

rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue May 30, 2019
CHANGES:

- Restricted the set of variables available for expansion in the destination
  filename of `install` stanza to simplify implementation and avoid dependency
  cycles. (ocaml/dune#2073, @aalekseyev, @diml)

- [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego,
  review by @diml, @rgrinberg)

- [coq] Add `coq.pp` stanza to help with pre-processing of grammar
  files (ocaml/dune#2054, @ejgallego, review by @rgrinberg)

- Add a new more generic form for the *promote* mode: `(promote
  (until-clean) (into <dir>))` (ocaml/dune#2068, @diml)

- Allow to promote only a subset of the targets via `(promote (only
  <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml)

- Improve the behavior when a strict subset of the targets of a rule is already
  in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes
  ocaml/dune#2061, @diml)

- With lang dune >= 1.10, rules in standard mode are no longer allowed to
  produce targets that are present in the source tree. This has been a warning
  for long enough (ocaml/dune#2068, @diml)

- Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and
  @aalekseyev).

- Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This
  allow to specify cookie requests from variables expanded at each invocation of
  the preprocessor. (ocaml/dune#2106, @mlasson @diml)

- Add more opam metadata and use it to generate `.opam` files. In particular, a
  `package` field has been added to specify package specific information.
  (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg)

- Clean up the special support for `findlib.dynload`. Before, Dune would simply
  match on the library name. Now, we only match on the findlib package name when
  the library doesn't come from Dune. Someone writing a library called
  `findlib.dynload` with Dune would have to add `(special_builtin_support
  findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml)

- Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125,
  @rgrinberg)

- Hide full command on errors and warnings in development and show them in CI.
  (detected using the `CI` environment variable). Commands for which the
  invocation might be omitted must output an error prefixed with `File `. Add an
  `--always-show-command-line` option to disable this behavior and always show
  the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg)

- In `dune-workspace` files, add the ability to choose the host context and to
  create duplicates of the default context with different settings. (ocaml/dune#2098,
  @TheLortex, review by @diml, @rgrinberg and @aalekseyev)

- Add support for hg in `dune subst` (ocaml/dune#2135, @diml)

- Don't build documentation for implementations of virtual libraries (ocaml/dune#2141,
  fixes ocaml/dune#2138, @jonludlam)

- Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg)

- Make `dune subst` add a `(version ...)` field to the `dune-project`
  file (ocaml/dune#2148, @diml)

- Add the `%{os_type}` variable, which is a short-hand for
  `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml)

- Allow `enabled_if` fields in `library` stanzas, restricted to the
  `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764,
  ocaml/dune#2164 @diml, @rgrinberg)

- Fix `chdir` on external and source paths. Dune will also fail gracefully if
  the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg)

- Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg)

- Run `ocamlformat` relative to the context root. This improves the locations of
  errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg)

- Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These
  would be undetected whenever the project was nested in another workspace.
  (ocaml/dune#2194, @rgrinberg)

- Fix generation of `.merlin` whenever there's more than one stanza with the
  same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg)

- Fix generation of `.meriln` in the presence of the `copy_files` stanza and
  preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206,
  @rgrinberg)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Jun 4, 2019
CHANGES:

- Restricted the set of variables available for expansion in the destination
  filename of `install` stanza to simplify implementation and avoid dependency
  cycles. (ocaml/dune#2073, @aalekseyev, @diml)

- [menhir] call menhir from context root build_dir (ocaml/dune#2067, @ejgallego,
  review by @diml, @rgrinberg)

- [coq] Add `coq.pp` stanza to help with pre-processing of grammar
  files (ocaml/dune#2054, @ejgallego, review by @rgrinberg)

- Add a new more generic form for the *promote* mode: `(promote
  (until-clean) (into <dir>))` (ocaml/dune#2068, @diml)

- Allow to promote only a subset of the targets via `(promote (only
  <pred>))`. For instance: `(promote (only *.mli))` (ocaml/dune#2068, @diml)

- Improve the behavior when a strict subset of the targets of a rule is already
  in the source tree for projects using the dune language < 1.10 (ocaml/dune#2068, fixes
  ocaml/dune#2061, @diml)

- With lang dune >= 1.10, rules in standard mode are no longer allowed to
  produce targets that are present in the source tree. This has been a warning
  for long enough (ocaml/dune#2068, @diml)

- Allow %{...} variables in pps flags (ocaml/dune#2076, @mlasson review by @diml and
  @aalekseyev).

- Add a 'cookies' option to ppx_rewriter/deriver flags in library stanzas. This
  allow to specify cookie requests from variables expanded at each invocation of
  the preprocessor. (ocaml/dune#2106, @mlasson @diml)

- Add more opam metadata and use it to generate `.opam` files. In particular, a
  `package` field has been added to specify package specific information.
  (ocaml/dune#2017, ocaml/dune#2091, @avsm, @jonludlam, @rgrinberg)

- Clean up the special support for `findlib.dynload`. Before, Dune would simply
  match on the library name. Now, we only match on the findlib package name when
  the library doesn't come from Dune. Someone writing a library called
  `findlib.dynload` with Dune would have to add `(special_builtin_support
  findlib_dynload)` to trigger the special behavior. (ocaml/dune#2115, @diml)

- Install the `future_syntax` preprocessor as `ocaml-syntax-shims.exe` (ocaml/dune#2125,
  @rgrinberg)

- Hide full command on errors and warnings in development and show them in CI.
  (detected using the `CI` environment variable). Commands for which the
  invocation might be omitted must output an error prefixed with `File `. Add an
  `--always-show-command-line` option to disable this behavior and always show
  the full command. (ocaml/dune#2120, fixes ocaml/dune#1733, @rgrinberg)

- In `dune-workspace` files, add the ability to choose the host context and to
  create duplicates of the default context with different settings. (ocaml/dune#2098,
  @TheLortex, review by @diml, @rgrinberg and @aalekseyev)

- Add support for hg in `dune subst` (ocaml/dune#2135, @diml)

- Don't build documentation for implementations of virtual libraries (ocaml/dune#2141,
  fixes ocaml/dune#2138, @jonludlam)

- Fix generation of the `-pp` flag in .merlin (ocaml/dune#2142, @rgrinberg)

- Make `dune subst` add a `(version ...)` field to the `dune-project`
  file (ocaml/dune#2148, @diml)

- Add the `%{os_type}` variable, which is a short-hand for
  `%{ocaml-config:os_type}` (ocaml/dune#1764, @diml)

- Allow `enabled_if` fields in `library` stanzas, restricted to the
  `%{os_type}`, `%{model}`, `%{architecture}`, `%{system}` variables (ocaml/dune#1764,
  ocaml/dune#2164 @diml, @rgrinberg)

- Fix `chdir` on external and source paths. Dune will also fail gracefully if
  the external or source path does not exist (ocaml/dune#2165, fixes ocaml/dune#2158, @rgrinberg)

- Support the `.cc` extension fro C++ sources (ocaml/dune#2195, fixes ocaml/dune#83, @rgrinberg)

- Run `ocamlformat` relative to the context root. This improves the locations of
  errors. (ocaml/dune#2196, fixes ocaml/dune#1370, @rgrinberg)

- Fix detection of `README`, `LICENSE`, `CHANGE`, and `HISTORY` files. These
  would be undetected whenever the project was nested in another workspace.
  (ocaml/dune#2194, @rgrinberg)

- Fix generation of `.merlin` whenever there's more than one stanza with the
  same ppx preprocessing specification (ocaml/dune#2209 ,fixes ocaml/dune#2206, @rgrinberg)

- Fix generation of `.merlin` in the presence of the `copy_files` stanza and
  preprocessing specifications of other stanazs. (ocaml/dune#2211, fixes ocaml/dune#2206,
  @rgrinberg)

- Run `refmt` from the context's root directory. This improves error messages in
  case of syntax errors. (ocaml/dune#2223, @rgrinberg)

- In .merlin files, don't pass `-dump-ast` to the `future_syntax` preprocessor.
  Merlin doesn't seem to like it when binary AST is generated by a `-pp`
  preprocessor. (ocaml/dune#2236, @aalekseyev)

- `dune install` will verify that all files mentioned in all .install files
  exist before trying to install anything. This prevents partial installation of
  packages (ocaml/dune#2230, @rgrinberg)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants