Add a (chdir ..) stanza

[rgrinberg]

This stanza allows one to write rules for a sub directory with the following
form:

(subdir bar ;; bar is the subdir
  ;; arbtirary stanzas can go here
 )

This form is quite useful for code generation. That's because it's easier to
generate the dune file in one go for an entire build rather than setting up
multiple rules.

It also gives us the ability to override dune file in subdirs. For example:

$ cat dune
(data_only_dirs foo)
(subdirs foo ..) ;; (1)
$ cat foo/dune
 <contents>

The idea is that should be ignored and the dune file is specified
using (1). This is quite useful in some vendoring situations where you don't
want to be applying your own patches.

[nojb]

What happens if there is already a dune file in the subdir which is not ignored ? Are the dune files merged ?

[rgrinberg]

What happens if there is already a dune file in the subdir which is not ignored ? Are the dune files merged ?

The current implementations behaves as follows:

• It concatenates all normal stanzas.
• Directory visibility stanzas such as (data_only_dirs .) override each other.

I realize that this isn't consistent, but it gives us the ability to shadow dune files in sub directories without editing them. That seems like a useful thing to have. We can also disallow stanzas such as (data_only_dirs .) inside (subdir ..)

[ghost]

The implicit shadowing behaviour makes me nervous. Are you thinking of shadowing dune files of vendored code? If shadowing is the intent, then it should be fully explicit to avoid surprises.

Otherwise, the feature in general seems good to me. What do you think of calling the stanza chdir? I have been thinking of a proper semantic for variable expansion, and it seems to me that chdir could be generalised so that it can appear pretty much anywhere.

[rgrinberg]

The implicit shadowing behaviour makes me nervous. Are you thinking of shadowing dune files of vendored code? If shadowing is the intent, then it should be fully explicit to avoid surprises.

How would explicit shadowing look like? Shadowing dune files of vendored dirs is indeed what I’m looking for. I agree that the way it’s working out here is quite confusing. I’m fine with removing this part of the PR for now and just erroring out whenever there are competing dirs, vendored_dirs, data_only_dirs stanzas for a directory.

Otherwise, the feature in general seems good to me. What do you think of calling the stanza chdir? I have been thinking of a proper semantic for variable expansion, and it seems to me that chdir could be generalised so that it can appear pretty much anywhere.

chdir seems like a better name. I will change it.

…

On Mar 16, 2020, 1:45 PM +0100, Jérémie Dimino ***@***.***>, wrote: The implicit shadowing behaviour makes me nervous. Are you thinking of shadowing dune files of vendored code? If shadowing is the intent, then it should be fully explicit to avoid surprises. Otherwise, the feature in general seems good to me. What do you think of calling the stanza chdir? I have been thinking of a proper semantic for variable expansion, and it seems to me that chdir could be generalised so that it can appear pretty much anywhere. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[ghost]

Not sure about the syntax, but essentially you would declare that you want to override competing stanzas. For instance:

(shadow_other_stanzas
 (dirs ...))

[nojb]

How would explicit shadowing look like? Shadowing dune files of vendored dirs is indeed what I’m looking for. I agree that the way it’s working out here is quite confusing. I’m fine with removing this part of the PR for now and just erroring out whenever there are competing dirs, vendored_dirs, data_only_dirs stanzas for a directory.

One possibility is for each chdir stanza to come with a merge_mode: merge (concatenate stanzas), replace (replace stanzas), etc.

[rgrinberg]

Actually, I'm not entirely sure I need anything special. The following should just work:

(data_only_dirs foo)
(chdir foo ..)

There's no need for any shadowing here.

[ghost]

Great, no shadowing is simpler :)

[rgrinberg]

@diml the implementation is now ready for review - there's no more shadowing.

Also, I found it quite difficult to reason about all the parent directories when computing the dune file. I've changed the algorithm to be a bit simpler by doing work upfront so that we need to look only at the dune file of the parent.

[rgrinberg]

Docs are ready as well.

cc @samoht @jordwalke this stanza could be quite useful for your dune file generation needs. Please see the docs.

[rgrinberg]

One possibility is for each chdir stanza to come with a merge_mode: merge (concatenate stanzas), replace (replace stanzas), etc.

I think the latest version of the PR covers this functionality in two steps. To replace, one may ignore the dune file in a directory using data_only_dirs, and then use chdir to set it manually. Otherwise, merging is the default behavior. See the tests for an example.

[nojb]

We just had a discussion offline and we discovered a slightly surprising behavior this feature introduces:

$ cat dune
(env (_ (env-vars FOO foo))
(chdir sub
 (rule (with-stdout-to bar (echo %{env:FOO=yyy})))
$ cat sub/dune
(env (_ (env-vars FOO bar)))

What should be the contents of bar in this case?

It should be bar I think -- chdir should be equivalent to writing the inner stanzas in the subdirectory, no ? It looks a bit strange, but I think that we may end up shooting ourselves in the foot if we try anything cleverer than that.

And if you want to make sure there is no shadowing of the environment variable, you would always do something like:

(with-stdout-to bar-env (echo %{env:FOO=yyy}))
(chdir sub
  (rule (with-stdout-to bar (cat ../bar-env))))

[rgrinberg]

Thanks for the feedback. @diml it seems like the other devs agree that it's most obvious to interpret the semantics as the code being in separate dune files. I tend to agree because I don't think it would be intuitive for fields in env to be lexically scoped. If we were to introduce a let stanza however, I would certainly expect it to be lexical.

[rgrinberg]

@diml I've updated the documentation to remove the shadowing example

[ghost]

Sounds good, let's go for this semantic then.

[ghost]

As discussed in the meeting, we should rename the stanza to something else than chdir given that the scoping of %{env...} is different between the chdir stanza and action.

[rgrinberg]

@diml will do.

@aalekseyev also volunteered to review the PR.

[aalekseyev]

Let me try to clarify my understanding of the overall design.

You're not changing the interface between Build_system and File_tree modules. From the point of view of Build_system, these "virtual" directories are treated exactly the same as a normal source directory that has a dune file in it, with no way to distinguish them using File_tree API.

If that's the right understanding, then the design seems good to me.

When I volunteered to review it's specifically the Build_system changes I was thinking of, and I approve all 0 lines of them. :-)

That said, one difference between virtual and real dirs that will be observable to the build system is the actual presence of the directory on disk. Don't we make some assumptions about the source directories actually existing on disk in some cases? One example I was thinking of is promotion: what happens if I write a rule that promotes into a virtual directory?

[aalekseyev]

Overall looks good to me. I haven't done the most thorough review possible, just glanced through the code, but the design seems good and the tests look good, and there's nothing glaringly terrible in the code.

[rgrinberg]

That said, one difference between virtual and real dirs that will be observable to the build system is the actual presence of the directory on disk. Don't we make some assumptions about the source directories actually existing on disk in some cases? One example I was thinking of is promotion: what happens if I write a rule that promotes into a virtual directory?

In case where this distinction is necessary, we can additional add al Virtual constructor to status. Regarding promotion, isn’t this the same as defining a promote rule with into set to a non-existent dir? We should handle these cases in the same way.

…

On Apr 1, 2020, 3:58 AM -0700, Arseniy Alekseyev ***@***.***>, wrote: Let me try to clarify my understanding of the overall design. You're not changing the interface between Build_system and File_tree modules. From the point of view of Build_system, these "virtual" directories are treated exactly the same as a normal source directory that has a dune file in it, with no way to distinguish them using File_tree API. If that's the right understanding, then the design seems good to me. When I volunteered to review it's specifically the Build_system changes I was thinking of, and I approve all 0 lines of them. :-) That said, one difference between virtual and real dirs that will be observable to the build system is the actual presence of the directory on disk. Don't we make some assumptions about the source directories actually existing on disk in some cases? One example I was thinking of is promotion: what happens if I write a rule that promotes into a virtual directory? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[aalekseyev]

The last 4 commits look good too.

[rgrinberg]

Thanks for the review. I've renamed chdir to subdir as we've discussed.