Path.is_in_build_dir test

[rgrinberg]

This function returns false on Path.build_dir which is somewhat surprising.

@diml, originally I thought this was just a bug, but it seems like we're relying on this to make sure that _build/.universe-state is built without any explicit rule for doing so. E.g:

let universe = Path.relative Path.build_dir ".universe-state"
assert (not (Path.is_in_build_dir (Path.parent universe)))

This seems a bit odd to me, but in case we should document this in a test if we indeed rely on this.

[ghost]

Hmm, that's odd indeed. Does anything break if you change Path.is_in_build_dir instead so that in returns true for Path.build_dir? For the universe file, this fact should come from this check

[rgrinberg]

Does anything break if you change Path.is_in_build_dir instead so that in returns true for Path.build_dir?

Yup, there's breakage if we try to change it:

+  Trying to build _build/.universe-state but build context .universe-state doesn't exist.

It comes from this check starting to fail for _build/.universe-state https://github.com/rgrinberg/jbuilder/blob/is-in-build-dir-weird-behavior/src/build_system.ml#L1043-L1044

For the universe file, this fact should come from this check

Hmm, it's a bit confusing that the build_dir is assumed to have no targets since clearly .universe-state is clearly one. Though, this isn't really the problem as we don't really have a build rule for the .universe-state file. In the current code, we are simply trying to trick the build system that this file should always exist. But this is the dependent on wonkiness of Path.is_in_build_dir that I've outlined.

[ghost]

It's not really that we don't have a rule for .universe-state. The build system works as follow:

• first we look if we have a rule for a given file in t.files
• if not we try to load the directory of the file first and look again

Loading a directory fills all the corresponding entries in t.files. For special files such as .universe-state, we eagerly fill t.files with the rule. The check in get_dir_status just means that trying to get a listing of the build directory will return an empty set of files.

Regarding this issue, I think we should do one of the following:

• rename Path.is_in_build_dir to make its semantic clearer. For instance Path.is_strict_descendant_of_build_dir
• change the behavior of Path.is_in_build_dir and change the test you pointed

[rgrinberg]

For special files such as .universe-state, we eagerly fill t.files with the rule

Where does this happen for .universe-state? All I see is writing the file.

change the behavior of Path.is_in_build_dir and change the test you pointed

This is option is preferable, but something else needs to change as well here. Just changing it breaks jbuilder in the way I've pointed above.

[ghost]

Ah, sorry, you are right, we just write the file directly.

This is option is preferable, but something else needs to change as well here. Just changing it breaks jbuilder in the way I've pointed above

Agreed

[rgrinberg]

I'm merging this for now to make sure this property doesn't change anywhere. I'll actually do the rename you suggested after #744 and then look for a proper fix.

[ghost]

Ok. BTW, I consider the rename to be a proper fix. The most important is that the code does what it looks like it does.