dune

dune copied to clipboard

Fixes #10378.

Initially I started looking into adding the changes at a later stage (Build_cmd, Report_error) but at that moment it seems it's already "too late" and I couldn't find a way to get the context where the process is being executed from. In the end I am resorting

The print function doesn't add any new information if the context name is default. Maybe there could be some opt-in configuration to enable default logging in all cases added in a future PR cc @davesnx.

Given the pervasiveness of User_message.make, I assume this initial PR is missing a lot of places where it'd be interesting to print the context name as well. But I thought it'd make things easier for reviewers to start with a small PR, and then follow up later on with other PRs that will include any improvements over it.

We're actually trying to get rid of contexts from the engine and make them strictly rule only, so this PR would make it more more difficult for us.

I think we pass the directory with some user messages. So you could try extracting in Report_error and deriving the context from that.

@rgrinberg is https://github.com/ocaml/dune/pull/10414/commits/63f17d133763693cfd77c048ae7aa960c136554a what you had in mind? I tried moving the with-directory annotation function to User_message to simplify things further, but this creates a cycle between User_message and Path. I assume that's the reason why it was in Process to begin with.

We're actually trying to get rid of contexts from the engine and make them strictly rule only, so this PR would make it more more difficult for us.

Ftr: https://github.com/ocaml/dune/issues/7049.

@rgrinberg is 63f17d1 what you had in mind? I tried moving the with-directory annotation function to User_message to simplify things further, but this creates a cycle between User_message and Path. I assume that's the reason why it was in Process to begin with.

I'm going to double check this tomorrow, but I think you can just get rid of that annotation and just include dir inside User_message.

I think you can just get rid of that annotation and just include dir inside User_message.

I tried adding a new field dir : Path.t option to User_message.t, but I get some dep cycle error:

Error: dependency cycle between modules in
_build/default/otherlibs/stdune/src:
   User_warning
-> Path
-> Path
-> required by _build/default/otherlibs/stdune/src/stdune.cmxa
-> required by
   _build/default/otherlibs/dune-rpc-lwt/examples/rpc_client/rpc_client.exe
-> required by _build/install/default/bin/dune-rpc-lwt-example-client
-> required by _build/default/dune-rpc-lwt.install
-> required by alias install

I don't get it very well because User_warning doesn't really use Path.

I think there's a cycle even without User_warning since in the original code:

• Path depends on User_error through User_error.raise calls
• User_error depends on User_message through the definiton of E

graph LR
User_error -- User_error.raise --> Path
User_message -- User_error.t --> User_error

That's a bit of a pain. I think you can just use a string path though.

I think you can just use a string path though.

Tried something in https://github.com/ocaml/dune/pull/10414/commits/550d20e5f33efdc58ff238cca727927ea30321db.

Tests are failing for unrelated reason, see https://github.com/ocaml/dune/pull/10484.

@pmwhite can you check if this change is going to work internally?

@rgrinberg I just wrote a patch to import this PR internally. It mostly applies without changes, except for a few places (e.g. action_runners.ml, process.ml, and build_system_error.ml) where we already had access to the directory through a job_summary field that has yet to be upstreamed. These places are not very significant, and all our internal tests pass, so this PR is :+1: with respect to not conflicting with internal changes.

Ok, thanks. In general, I'm quite happy to get rid of this "annots" map and just shove things directly into the user error type. Once the internal patch is merged, could you sync this PR with it and we can merge?