[fix] [directory targets] Fix promotion of directory targets in some cases

[ejgallego]

When the promoted directory has sub-directories with only other sub-directories inside, the map will miss that directory and it will not be properly re-created, failing at children creation time.

I am not sure I fully follow the logic here (cc: @rleshchinskiy).

The current test case works for me (TM), if we revert the fix we get:

Error: Cannot promote files to "node_modules/node-cmake/node_modules/ansi-regex".
Reason: opendir(node_modules/node-cmake/node_modules/ansi-regex): No such file or directory

Regression in b1c339b / #9407

Closes #10609 .

[moyodiallo]

After digging a bit, I found the issue could come from:

dune/src/dune_engine/target_promotion.ml

Lines 210 to 225 in 1a8b43f

	(* There can be some files or directories left over from earlier builds, so we
	need to remove them from [targets.dirs]. *)
	let remove_stale_files_and_subdirectories ~dir ~expected_filenames =
	(* CR-someday rleshchinskiy: This can probably be made more efficient by relocating
	root once. *)
	let build_dir = Path.Build.append_local targets.root dir in
	let dst_dir = relocate build_dir in
	(* We use a tracked version to subscribe to the correct directory listing.
	In this way, if a user manually creates a file inside a directory target,
	this function will rerun and remove it. *)
	Memo.run (Fs_memo.dir_contents ~force_update:true (In_source_dir dst_dir))
	>>|
	let dst_dir = Path.source dst_dir in
	function
	| Error unix_error -> directory_target_error ~unix_error ~dst_dir []
	| Ok dir_contents ->

The function is supposed to remove the stale files and sub-directories, but at the line 224, we are not supposed to report the error when the source directory doesn't exist which is fine anyway.

You should observe the effect replacing the line by

  | Error unix_error -> ()

but it's not end fix, because we could have other type of errors.

[moyodiallo]

I'm not sure for this fix, I think this code is fine. The issue is coming from somewhere else.

[ejgallego]

Thanks a lot for the help. My impression is that indeed it matters that we add the binding { dir -> [] } to the map, as even if the directory doesn't have files, it can have subdirs.

But I didn't track down exactly the part that makes the current code break.

[ejgallego]

So indeed you are correct that the crash happens at remove_state_files_and_subdirectories, but my impression is that we break the invariant about source representation that the above function requires.

[moyodiallo]

I get it, that's fair point.

The question I have now is, are we going to allow promoting empty directories ?

[ejgallego]

Maybe the confusion is about what we call "empty directory", empty directories are OK not to promote in my view, however here the directories in question are not empty, they just have no files, however they do have other subdirectories, so definitively they need to be considered part of the sources.

[moyodiallo]

I tested the fix, it is promoting even empty directories.
This is not fixing exactly the test-case, it's fixing it indirectly by allowing an empty directory.

[ejgallego]

Indeed after this, empty directories are promoted. I don't see why they shouldn't be promoted?

We could optimize this away, but I am not sure it is worth it, it would likely require more invasive changes. Note that in the PR that caused this regression, not promoting empty dirs was regarded as an optimization IIRC.

Note that the fix doesn't exactly allow "empty directories", it allows too, crutially, directories that have no files but are not empty, such as the test case.

[moyodiallo]

Looking to this further. I agree with you, this where we need to fix the issue.

[moyodiallo]

This is one of the solution moyodiallo@b00b92e.

The thing is adding all the parent directories of the collected files, it will ends up with no empty directory.