Tweak mkdir_p uses

[rgrinberg]

If the directory that we're trying to create exists, then this is never an error.

We now forbid actions to create external or source dirs.

This should fix #2158

[rgrinberg]

It's a bit unfortunate that we can't give the user a proper location here, but passing the location along would be quite painful

[ghost]

One sec, where does such an action come from? The mkdir action is not exposed in the action DSL.

[ghost]

Additionally, the check_mkdir in action_unexpanded.ml should catch this

[rgrinberg]

Additionally, the check_mkdir in action_unexpanded.ml should catch this

Ah yes, then we can just turn this check into an assert I think.

One sec, where does such an action come from? The mkdir action is not exposed in the action DSL.

From the chdir I believe.

[ghost]

Ah yes, then we can just turn this check into an assert I think.

Seems good. Or a Exn.code_error.

From the chdir I believe.

These are handled in build_system.ml. It must be from somewhere else

[aalekseyev]

Do you have a motivating example? At a first glance, it seems to me that it makes the logic more complicated for no obvious gain.
Sorry, I can't read.

[rgrinberg]

These are handled in build_system.ml. It must be from somewhere else

The error from the bug report indeed comes from build_syste.ml. In particular, Mkdir_p.exec:

  let exec p =
    if Path.is_managed p then
      Memo.exec def p
    else
      Exn.code_error "Mkdir_p.exec: attempted to create unmanaged dir"
        [ "p", Path.to_sexp p
        ]

So the first commit will fix the bug for sure.

The second commit was a preventative measure after reviewing more uses of mkdir. It's not necessary to fix the bug.

[rgrinberg]

Do you have a motivating example? At a first glance, it seems to me that it makes the logic more complicated for no obvious gain.

The first commit fixes a bug reported in #2158

The second commit can be omitted, but I think it would be useful to turn it into Exn.code_error

[aalekseyev]

It would be nice to have a test for this because I imagine that use case is very easy to forget about and break in the future.

[rgrinberg]

I'll add a test case.

[ghost]

Turning it into a code error seems good. However, Dune should never try to create directories outside of the workspace. So I feel like we are hiding a real problem by silently ignoring external mkdirs when the external path exists.

[ghost]

From the chdir I believe.

These are handled in build_system.ml. It must be from somewhere else

Sorry, I read the code wrong, I thought this was touching action_exec.ml. However, looking at the stacktrace, you are right that this come from a chdir. What we should do is not call Mkdir_p for external chdirs. For instance, we could replace Action.chdirs by Action.managed_chdirs that would only return managed paths.

[rgrinberg]

What we should do is not call Mkdir_p for external chdirs. For instance, we could replace Action.chdirs by Action.managed_chdirs that would only return managed paths.

Managed? If we're restricting this, I think we should go all the way and only allow build dirs. Chdir'ing into the source dir seems dangerous to me.

Just to confirm: #2158 is not considered to be a regression, and that we indeed don't allow to chdir into external paths.

[rgrinberg]

I removed the "fix" which ignored the mkdir_p on external dirs when the dir already existed. All that remains is the assertion that we're only creating a build dir in the action. I think we can safely merge that.

[ghost]

Seems good to me