Fix the priority order for environment variables between workspace and context

[bobot]

Use env-vars from the dune-workspace for dune use.

[bobot]

@nojb Perhaps you can take a look.

[bobot]

I propose to change that relative path are absolutize with respect to the build directory instead of the source directory, since it is where things should be. It forces to have the right dependencies.

[nojb]

I propose to change that relative path are absolutize with respect to the build directory instead of the source directory, since it is where things should be. It forces to have the right dependencies.

I am not sure about this; if I understand correctly this is the right thing to do only if the binaries are being managed by dune.

Suppose that I have a toplevel tools directory with some assorted binaries that I want to bring into the PATH during by build. Before this change, dune-workspace had

(paths (PATH tools))

after this change it will have be

(paths (PATH ../../tools))

is that right?

[bobot]

The dune-workspace is the same, you just add a (deps (source_tree tools) to the rules which uses these binaries. So that dune copy the binaries in the directory _build/default/tools and record the dependency.

[nojb]

The dune-workspace is the same, you just add a (deps (source_tree tools) to the rules which uses these binaries. So that dune copy the binaries in the directory _build/default/tools and record the dependency.

This won't work for the compiler tools for example, which is one of the use-cases of #2426 .

[bobot]

This won't work for the compiler tools for example, which is one of the use-cases of #2426 .

Oh, I though your compiler tools were in absolute directories. Sorry. In your case does (paths (PATH %{workspace_root}/tools)) is possible?

[nojb]

This won't work for the compiler tools for example, which is one of the use-cases of #2426 .

Oh, I though your compiler tools were in absolute directories. Sorry. In your case does (paths (PATH %{workspace_root}/tools)) is possible?

I will give it a try, but I don't think we currently expand variables when interpreting paths (or env-vars for that matter), but it may end up being the best solution.

[nojb]

Oh, I though your compiler tools were in absolute directories. Sorry. In your case does (paths (PATH %{workspace_root}/tools)) is possible?

I gave it a try; indeed variables are not expanded so that would need fixing, but by writing an absolute path by hand it seems to work fine. Do you think you could look into the issue of variable expansion?

[bobot]

Now the variable is expanded. However since we are very early, the context record is not yet built and so I couldn't use Pform. So it is perhaps not the right way but it works.

[nojb]

Now the variable is expanded. However since we are very early, the context record is not yet built and so I couldn't use Pform. So it is perhaps not the right way but it works.

We probably want to do this properly, so I would be OK if you merge your PR without the expansion of variables, and we look at that properly in a later PR.

[bobot]

Ok so I removed the last commit and created a new merge request with the last commit (not based on this PR)

[ghost]

I haven't read this PR in detail, but just one important detail to keep in mind: currently, when Dune looks up a binary in the PATH, it only test for the existence of the file via a plan Sys.file_exists. In particular, it doesn't go through the build system.

This is obviously incorrect if the PATH contains directories that point inside the build directory, as the result would depend on the time at which dune happened to call Sys.file_exists. Up to know, we made the assumption that all components of PATH were pointing outside the build directory.

Reading the discussion, I get the feeling that this PR allows to have parts of PATH point to the build directory. Is that the case?

[rgrinberg]

You need $ make fmt here

[rgrinberg]

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

[nojb]

Reading the discussion, I get the feeling that this PR allows to have parts of PATH point to the build directory. Is that the case?

@bobot will be able to give the definite answer, but yes, I think that all relative paths will now be interpreted relative the build directory.

[ghost]

Ok, so that cannot work in the current world. We could change the way dune resolves binaries to make it work (which would be really nice!), but in the meantime components of PATH cannot point to the _build directory.

[rgrinberg]

@bobot please re-open if you rebase.