Shouldn't we declare dependencies on specific variables rather than the whole environment? Depending on the whole environment seems safer, but then Dune should probably cleanup a bit the environment and assume that basically everything depend on it.

the environment is tracked per context. Does it make sense to use the current context, or is "default" (as in the current patch) enough?

It should be per context. There are many variables that might influence the build that are per-contexts, such as OCAMLPATH.

Shouldn't we declare dependencies on specific variables rather than the whole environment?

I saw that as a possible improvement, but you're right, in most cases a list will be enough. I'll change that.

It should be per context

👍 fixed this

I'm having trouble doing this per environment variable.

What I'm trying to implement is having a per-context, per-var file that consists of the variable content (say, "unset" or a hash of the value). I'd like to store that in _build/.env/$CONTEXT/$VAR.

The problem is how to populate these files: it's not possible to do it in Build_system.do_build because we don't know the set of relevant variables at this point. How would it be possible to do this lazily, one a target requires it?

Thanks!

The most straightforward way is to allow rules to have other kind of dependencies than just files. This is a bit more work, however that would help for other cases. Knowing precisely what the dependencies of a rules are is better than encoding special dependencies via files.

For instance, currently we represent dependencies on /some/externa/dir/*.cmi via a special target in _build/.misc. Such files might appears in dependency traces, in the output of dune rules, etc... This is also a problem for #1171.

We don't need to change everything in this PR, but we can discuss how to proceed in a way that will help with future changes. For instance we could replace the various Path.Set.t values that are used to represent dependencies by an abstract type Dependencies.t, and then allow Dependencies.t to encode environment variable dependencies.

Thanks for the suggestion. I just did that. I'm not sure that this covers all uses of Path.Set.t for dependencies, but at least my test case now passes.

There seem to be quite a few cases where we call Dependencies.iter or Dependencies.fold with on_var:ignore or on_var:(fun _ acc -> acc), which means that we do unnecessary work. This is especially annoying for parallel_iter.

I suggest to either add specialized iter_paths functions, or simply one paths : t -> Path.Set.t function and operate on the result.

I added some specialized versions for paths (it turns out that variables are just used for trace). I think it's preferable to adding a paths functions, because otherwise it's easy to just store a path set when the full Dependencies.t value was necessary later.

I'm a bit surprised that we needed to modify the build arrow to support this feature. I would have thought that doing it via vfile's would have been enough.

Btw, one thing that I'm wondering about is that if we should be conservative with the env vars we expose to actions. This is a massively breaking change, but I think it would make more sense if actions did not see any environment unless they explicitly depended on it. This is nix inspired, and it makes it much easier to track all dependencies in builds correctly.

I would have thought that doing it via vfile's would have been enough.

That was my thought as well, but this has some drawbacks:

• hashing the whole environment works, but you can't track an individual variable (that was my first implementation)
• dumping the whole environment to individual files seems wasteful and is potentially a security issue.
• it's possible to create the files when the environment is accessed, but this requires changing the build arrow, so then why touch the FS at all?

The Dependencies.t abstraction suggested by @diml seems to be the right one. If you're concerned with adding another constructor to Build.t, maybe we can consolidate the parts of the build arrow that touch paths and env vars so that they're expressed in terms of this abstraction instead?

Btw, one thing that I'm wondering about is that if we should be conservative with the env vars we expose to actions.

Yes, that's something we should consider for dune 2.0. It would make reproducibility easier as well.

Yeah, I was just wondering about what we'd have to do if we wanted to add yet another type of dependencies. Consider if we wanted to add a dependency type for keys in a configuration file. Would we have to extend the build arrow again? Anyway, this is more of abstract issue, it's definitely not a blocker for this PR.

Btw, what about renaming Dependencies to Deps? I know this is a matter of taste, but Deps is a name that we use in many other places already.

Tightly controlling the environment would be ideal yes. Note that the same problem exist for files: we declare what files we think a given command is going to read, but we still give it the full view of the file system. However, for environment variables it's easier to restrict the view without loss of performances.

• It turns out that a lot of diff/union operations were just passed to an iteration function, so I added some specialized functions
• I renamed the module to Deps
• I also extracted Static_deps into its own module, with an abstract type

I added some documentation for the new modules. Let me know if that works for you.

About the "trace" functions: I think it would be possible to add an abstract type (that's just Obj.t in disguise) with a monoid operation like let mix a b = Obj.repr @@ Marshal.to_string (a, b) and replace the use of Marshal by this. Do you think it would be useful? I can try that in a separate PR.

Looks good. For the trace that's seems good yes, however we should call Marshal.to_string only once when we want to compute the digest, for performance reasons.