[RFC] Allow dune to treat targets opaquely.

[jordwalke]

Summary: Some targets will be generated by external build commands (such
as static resource managers, documentation generators, etc). Often,
those resources must be generated into a directory and the external tool
will manage the contents of that directory.

Dune can properly determine when to invoke the external build tool to
regenerate that directory content, and the external build tool will be
responsible for doing so.

In this sense, Dune can treat directories generated by build commands as
"opaque" resources, almost as if the directory was the tar equivalent of
the directory.

This diff attempts to implement this, but I'd like some feedback.

There is one issue with this implementation which I could use some help
resolving. Even if the mtime does not change for the file, Dune decides
to regenerate the target - merely because it is a directory, and I
cannot find the place in Dune where it decides to do that.

[ghost]

The idea is essentially to add support for directory targets. This seems fine, especially as we currently do it for odoc but in a not very clean way. When considering the digest of a directory, we should probably consider the digest of the contents of the directory as well. Otherwise, dune will not re-generate it if it is modified.

Regarding the current problem you are seeing, have you tried printing the stats to see if something is changing?

[jordwalke]

Regarding the current problem you are seeing, have you tried printing the stats to see if something is changing?

Yes, and something is changing the mtime of them - and it's not me. But I think the mtime is being changed because Dune is regenerating it. I don't know why it's regenerating it. Is there any place you can think of where Dune would care that this is a directory vs. file and therefore remove it and/or regenerate it?

When considering the digest of a directory, we should probably consider the digest of the contents of the directory as well. Otherwise, dune will not re-generate it if it is modified.

My understanding was that Dune will regenerate it if its dependencies are modified, right? I think that factoring the contents of the directories would be useful if those directories are inputs to other targets, right? I was even content with only supporting directories at root of dependency trees, because the common case is external systems that generate docs etc. They aren't typically used as inputs to other rules. Thoughts?

[ghost]

Yes, and something is changing the mtime of them - and it's not me. But I think the mtime is being changed because Dune is regenerating it. I don't know why it's regenerating it. Is there any place you can think of where Dune would care that this is a directory vs. file and therefore remove it and/or regenerate it?

Not that I can think of. It feels like Dune should really care about whether a target is a directory or a file, though I can't think of a place where this matters at the moment.

My understanding was that Dune will regenerate it if its dependencies are modified, right?

Since #1343, it will also regenerate it if the target is modified.

I think that factoring the contents of the directories would be useful if those directories are inputs to other targets, right?

Yes.

I was even content with only supporting directories at root of dependency trees, because the common case is external systems that generate docs etc. They aren't typically used as inputs to other rules. Thoughts?

That's fine as long as we get a proper error message if we try to use such a directory as dependency.

[jordwalke]

For example, even if I change the dir_digest function to always return the string of the file path (which should never change), Dune will still rebuild it from scratch. So for some reason being a directory makes Dune think it changed.

[ghost]

Ah indeed, that's the function remove_old_artifacts in build_system.ml that deletes it.

[ghost]

This function should be adapted to support directory targets

[jordwalke]

I made the change you suggested. It seems to work.

[ghost]

We can use normal equality here instead of physical one:

Suggested change

	if stat.st_kind == Unix.S_DIR then
	if stat.st_kind = Unix.S_DIR then

[ghost]

It looks like we could factorise this bit of code between here and refresh

[ghost]

Thanks, could you sign your commits as per the CONTRIBUTING.md file and add a changelog entry?

We'll need to update dir_digest so that it takes into account the contents of the directory, but this can be done in another PR.

[jordwalke]

Suggestions applied, signed off.

[ghost]

Thanks. js_of_ocaml doesn't seem to build in travis, we need to double check if it builds with master or if the build of jsoo is broken by this PR.

[ghost]

I had a quick look and couldn't reproduce the error locally. I restarted the build.

[ejgallego]

I've seen this error in other contexts when composing builds after #1554 , so indeed I have been working with a pre #1554 tree locally.

[ghost]

@ejgallego do you have a full backtrace? It is truncated in the CI logs

[ejgallego]

@ejgallego do you have a full backtrace? It is truncated in the CI logs

Let me try to reproduce.

[ejgallego]

For some reason I am not able to reproduce reliably but it looks like this:

Description:
("Build_system.get_collector called on closed directory"
 (dir (In_build_dir install/default/bin))
 (load_dir_stack ()))
Backtrace:
Raised at file "src/stdune/exn.ml", line 32, characters 5-10
Called from file "src/build_system.ml", line 1589, characters 18-47
Called from file "src/install_rules.ml", line 217, characters 6-77
Called from file "list.ml", line 99, characters 22-25
Called from file "src/stdune/list.ml", line 5, characters 19-33
Called from file "src/install_rules.ml", line 321, characters 6-108
Called from file "src/install_rules.ml", line 342, characters 6-22
Called from file "map.ml", line 295, characters 20-25
Called from file "map.ml", line 295, characters 10-18
Called from file "src/gen_rules.ml", line 234, characters 4-25
Called from file "map.ml", line 295, characters 20-25
Called from file "src/gen_rules.ml", line 308, characters 2-60
Called from file "src/fiber/fiber.ml", line 111, characters 22-27
Called from file "src/fiber/fiber.ml", line 299, characters 6-13

[ghost]

Thanks, I think I see the issue

[ghost]

Anyway, we can merge this PR since the build failure is due to something else.

[ghost]

@jordwalke, thanks for your contribution!

[ejgallego]

Shouldn't this PR have also updated doc/ ?

[ghost]

Before advertising this feature more, we need to:

• implement proper hashing of directory, otherwise depending on a directory will not have the desired effect
• allow to declare that a target is a directory, so that if a rule A produces directory foo and another rule B requires file foo/bar, then Dune knows that A produces foo/bar.

[ejgallego]

Cool, thanks @diml, should something be done about (install ...) too?

[ghost]

Yh, we should be able to install a directory. Also that's quite a bit more work.

[rgrinberg]

How do we handle $ dune build dir now? Before this would mean just to build $ dune build @dir/all, but now it should mean build the dir target if it exists.

[ghost]

Having dir both as a target and present in the source tree doesn't seem compatible, so the current behavior of dune build dir is indeed:

• build directory dir if it is a target
• build @dir/all if dir is present in the source tree

That said, we should probably add a check somewhere to make sure we don't declare an existing directory as target.

[avsm]

It's a relatively minor thing, but (a ^ b ^ c) will allocate an intermediate string (a^b) that creates unnecessary garbage. It might be better to do a sprintf here to do it in one allocation if the digest function is called a lot

[ghost]

Now that you point that out, I just realise that we are missing separators in this string. i.e. two different stats records might produce the same string... That's very unlikely, but just to avoid readers wondering about it, we should add separators

[rgrinberg]

Fix here #1592

[jordwalke]

thank you all!

[cpitclaudel]

Having dir both as a target and present in the source tree doesn't seem compatible

But what about a promoted directory? Wouldn't that end up in the source tree and as a target?

[ghost]

FTR, we have been working on proper support for directory targets, including promoted directory targets. The support is almost complete in the main branch. It is still guarded by (using directory-targets 0.1) until the feature is mature enough.