Vendored code support

[NathanReb]

Fixes #2259, fixes #2260 and fixes #1016.

This PR adds a stanza to allow one to mark directories as containing vendored code so they can be
treated accordingly by dune.

When resolving aliases, such directories are not traversed. That means that by default installable
targets won't be built in there (unless required by an actual target), tests won't be run, code
won't be formatted or linted and so on.

Anything in a vendored directory is built with the release profile and any warnings are silenced to
prevent pollution of the standard output.

Note that the above behaviour doesn't apply if the user explicitly asked for a target within a
vendored dir.

[NathanReb]

This is just a draft for now and only the alias resolution part is implemented. I added blackbox
tests to help understand the above described behaviour. They use a duniverse-shaped project but
I'll try to add simplified test cases later eventually. I mostly wanted to get some feedback and
make sure I was on the right tracks as I have some concerns with my current implementation.

I went for a vendored_dirs stanza instead of vendor because it has a syntax and behaviour
similar to data_only_dirs but I'm happy to change that.

The way it currently works is that File_tree.Dir.t now has a status field which can be
Data_only, Vendored or Normal instead of the previous ignored boolean field.
I added an extra traverse_vendored_sub_dirs to File_tree.fold that, as the name suggests,
tells whether to descend into sub directories marked as vendors or not. That argument is set to
false in Build_system.Alias so that the resolution works as expected. It is also set to false
in Upgrader to prevent from upgrading jbuild files in vendored code by default.

As I said my concerns are mostly about the implementation and not so much about the current
behaviour:

• Currenlty, in a file tree, only the root of a vendored directory is marked as so instead of
  recursively marking it and all of its content, as it is done for data-only dirs. The main
  reason for that is to keep it easy enough to implement the behaviour when an vendored directory
  is passed explicitly on the command line.
• The current status thing seems to work well but I'm not sure how data-only and vendored dirs
  should work together. I think it should support ones within the others as vendored marking is
  pretty lightweight and data-only as precendence over everything else anyway.
• The custom behaviour of File_tree.fold when being passed a vendored dir might be a little
  confusing although I documented it.

If the approach seems good to you I've already noted a few things that I wanted to do but skipped
to get a first prototype ready and get some feedback ASAP:

• Forbid users from marking the same dir as both data-only and vendored in a dune file.
  I wanted to do this at first but I thought it would be better to be able to report the location
  of the error and thus to make Sub_dirs.eval take a (Loc.t * Predicate_lang.t) t. It should
  work fine since it's only used in File_tree
• Make Sub_dir.Status.t and File_tree.Dir.status polymorphic variants so we can drop the
  conversion function and its assert false.
• Version the new stanza to >=1.11
• Add some simpler tests, eventually remove the duniverse based ones

Next is the profile thing and if possible the warning one. I think the profile one is essential
and should be done in this PR but silencing the warning can probably be done separately.

Let me know what you think!

[rgrinberg]

Make data_only_dirs and vendored mutually exclusive seems fine to me. I agree that data only should take precendence over pretty much everything else.

The main reason for that is to keep it easy enough to implement the behaviour when an vendored directory is passed explicitly on the command line.

I suggest not to worry about this for now. Ignoring, data only, and vendoring is something that will need to be controlled via workspace or the command line. There's no need to specifically solve this problem for vendoring.

[rgrinberg]

I wanted to do this at first but I thought it would be better to be able to report the location of the error and thus to make Sub_dirs.eval take a (Loc.t * Predicate_lang.t) t. It should work fine since it's only used in File_tree

Perhaps we should just include the loc for decode predicate_lang's? It's quite annoying to keep track of this manually and remember to pass this around.

[rgrinberg]

Make Sub_dir.Status.t and File_tree.Dir.status polymorphic variants so we can drop the conversion function and its assert false

How about the following instead:

module Status : sig
  type t = Data_only | Normal | Vendored
  module Or_ignored : sig
    type nonrec t = Ignored | Status of t
  end
end

we don't really use polymorphic variants in dune. This use case doesn't seem like a strong enough reason to introduce them.

[NathanReb]

I suggest not to worry about this for now. Ignoring, data only, and vendoring is something that will need to be controlled via workspace or the command line. There's no need to specifically solve this problem for vendoring.

Yeah I think you're right, my reasoning was that I didn't want it to be a world of pain for duniverse users to toy around with vendored code but I guess

dune buid @alias --root vendor/some-package

works just as good and simplifies the code. I guess the only thing that it won't let you do is:

dune build @alias vendor/some-package vendor/some-other-package

but I guess that's fine for now.

we don't really use polymorphic variants in dune

Yeah I just realised that after some git greping. Your solution looks good, I'll do that!

[NathanReb]

I haven't thought about it until now but what should we do about .merlin files in vendored dirs?

My guess would be not to generate them but they can be quite convenient when browsing code sometime so I'm not so sure. I guess one can always force their generation by building the libraries on its own using the --root option but it could become quite tedious.

I realized that because I tried running dune clean and for some reason it didn't remove the .merlin files in my vendored directory, which is something that I need to sort out if we decide to generate them.

[NathanReb]

Hmm ok nevermind the last bit. The unremoved .merlin thing seems to be unrelated to the vendored_dirs stanza.

[NathanReb]

I pushed some new commits!

I removed the custom behaviour when explicitly specifying vendored directories to dune build on the command line as you asked.

Now vendored code is built with a -w -a flag to disable all warnings. To do that I flagged directories as vendored recursively in File_tree and Super_context.ocaml_flags now appends the above mentioned warning disabling flag is the given dir argument describes a vendored directory in the SC's file_tree.

I'm not familiar enough with the flag computation logic but that seemed like the right place to do so without overwriting some other important flags.

I think the only things left are:

• Version the new stanza to >=1.11
• Add an error when the user tries to mark a dir as both vendored and data-only

Let me know what you think!

[NathanReb]

I should also probably add a bit of documentation.

[rgrinberg]

Generating the .merlin files in the vendored dirs certainly seems desirable. It would be quite nice to be able to jump to vendored code automatically.

[avsm]

+1 to merlin files in vendored dirs! I'm using this patch in the dune-universe/opam-overlays CI to generate static binaries of vendored ocamlformat and duniverse itself. Hurrah!

[rgrinberg]

@NathanReb looking good so far. I believe this is no longer a draft.

Do you think you can change the dune code base to use this new vendored stanza? We currently have a vendor directory with vendored code.

[NathanReb]

Do you think you can change the dune code base to use this new vendored stanza?

Yeah sure!

I think it's indeed ready, except for that data-only + vendored error. I tried to do it naively and it turned out to be a bit tedious so I'll try to think of something else but I guess it's okay if we merge it with a non located error at first.

@emillon had an interesting suggestion regarding Sub_dirs related stanzas. In 2.0 we could change that for a single stanza with include, vendored and data_only fields. That doesn't solve everything but then everything is under the same location and I think it would be clearer to the users they have to be exclusive, something like:

(dirs
 (include :standard)
 (data_only :standard data-only-* examples)
 (vendored :standard vendor))

I'm just writing this down here for future reference, not intending to implement it right now.

I'm undrafting but I'll still work on:

• documenting the stanza
• adding an unlocated error when a dir is marked as data-only and vendored
• use it in dune's codebase

[rgrinberg]

The suggestion seems fine to me. Can this be done in a backwards compatible way however? It really is just an "aesthetic" change, so I think users might not be too happy if their stanza breaks for no tangible benefit.

[NathanReb]

I tried adding a dune file at the root of the project with:

(vendored_dirs vendor)

But unfortunately it seems like the predicate language isn't available during bootstrap, see the error:

$ make
./boot.exe
Error: exception Failure("globs are not available during bootstrap")
Backtrace:
Raised at file "stdlib.ml", line 33, characters 22-33
Called from file "src/glob.ml", line 16, characters 2-30
Called from file "src/glob.ml", line 23, characters 8-22
Called from file "src/dune_lang/dune_lang.ml", line 713, characters 24-30
Called from file "src/dune_lang/dune_lang.ml", line 601, characters 19-30
Called from file "src/dune_lang/dune_lang.ml", line 601, characters 19-30
Called from file "src/dune_lang/dune_lang.ml", line 598, characters 19-30
Called from file "src/dune_lang/dune_lang.ml", line 851, characters 20-32
Called from file "src/dune_lang/dune_lang.ml", line 1012, characters 25-39
Called from file "src/dune_lang/dune_lang.ml", line 1087, characters 19-30
Called from file "src/dune_lang/dune_lang.ml", line 1086, characters 19-30
Called from file "src/dune_lang/dune_lang.ml", line 601, characters 19-30
Called from file "src/dune_lang/dune_lang.ml", line 1060, characters 23-63
Called from file "src/dune_lang/dune_lang.ml", line 779, characters 19-28
Called from file "src/dune_lang/dune_lang.ml", line 719, characters 24-53
Called from file "src/dune_lang/dune_lang.ml", line 693, characters 15-29
Called from file "src/file_tree.ml", line 68, characters 12-111
Called from file "src/stdune/exn.ml", line 11, characters 8-11
Re-raised at file "src/stdune/exn.ml", line 13, characters 36-37
Called from file "src/file_tree.ml", line 292, characters 16-171
Called from file "camlinternalLazy.ml", line 27, characters 17-27
Re-raised at file "camlinternalLazy.ml", line 34, characters 10-11
Called from file "src/file_tree.ml", line 127, characters 20-32
Called from file "src/file_tree.ml", line 156, characters 22-34
Called from file "src/dune_load.ml", line 261, characters 4-303
Called from file "src/main.ml", line 44, characters 4-58
Called from file "src/main.ml", line 227, characters 10-145
Called from file "src/fiber/fiber.ml", line 99, characters 6-13

I must not segfault.  Uncertainty is the mind-killer.  Exceptions are
the little-death that brings total obliteration.  I will fully express
my cases.  Execution will pass over me and through me.  And when it
has gone past, I will unwind the stack along its path.  Where the
cases are handled there will be nothing.  Only I will remain.
make: *** [Makefile:9: default] Error 1

Should I make a small subset of re available during bootstrap so that we can at least use plain strings in there?

[rgrinberg]

You can use the same hack we use for the data only dirs and the examples directory:

          let status =
            if Bootstrap.data_only_path path then
              Sub_dirs.Status.Ignored
            else
              Sub_dirs.status sub_dirs ~dir:fn
          in

[rgrinberg]

@NathanReb did you forget to promote?

[NathanReb]

I think I did !

[NathanReb]

You can use the same hack we use for the data only dirs and the examples directory

It seems to be a bit trickier for the vendored directories as they still need to be traversed by the bootstrapped dune meaning I can't hide the (vendored_dirs ...) field within the vendor directory, as is done for test and example.

I might be missing something though!

[rgrinberg]

Hmm, we might be talking about something else. I was thinking that you would add a function like:

val vendored_path : Path.Source.t -> bool

and would hardcode it to the vendor dir in the bootstrap implementation (bootstrap.boot.ml).

[NathanReb]

Ok, I removed the commit!

Last remaining things I believe are:

1. Make the vendored_dirs stanza available as an extension
2. Silence some dune warnings in vendored directories such as jbuild files warnings or inaccurate .merlin

1 is easy enough but there doesn't seem to be proper support for 2 in dune so it will probably take longer. Might be worth considering merging this before.

What do you think?

[ghost]

What's the issue with 2?

[rgrinberg]

Personally I think that even 1) can be skipped. I think we are leaning too much towards the pessimistic side without much justification. We didn't go through any of this process with dirs, ignored_subdirs and other similar features and things turned out fine. The dune language was sufficient to allow a smooth transition. With this new stanza, I don't see a reason why we might not be able to support it in future versions of dune in its current form. So in the end:

• This is making a maintainer do extra work upfront.
• It introduces inconvenience for users. They might not want to adopt this stanza for fear that it might mean their build will break in the future.

While the benefits seem dubious. I'd say this another case of YAGNI

[rgrinberg]

What's the issue with 2?

There should be no issue. I think you should set ~allow_approx_merlin based on the status of the current dir.

[ghost]

I just learned about YAGNI :) In my mind, one reason to mark this experimental initially is the following: when we discussed about it, we were not sure whether it was better to use a vendor_dirs stanza or a dune-vendor file at the root of the vendored directory. Both have pros and cons.

If we go ahead with the vendor_dirs stanza, release it as part of dune 1.11 and realise one week after we started using the new feature that actually the dune-vendor file was better, then we are in an uncomfortable situation.

Marking the feature as experimental initially is relatively cheap in terms of additional development time and gives us a bit more time to make small breaking adjustments without having to think about backward compatibility.

[rgrinberg]

I think I came off harsher than I intended. If there are concrete breaking changes that are being considered, then making it experimental is obviously desirable. But doing it because we are trying to preempt something unknown is not enough of a reason.

If we go ahead with the vendor_dirs stanza, release it as part of dune 1.11 and realise one week after we started using the new feature that actually the dune-vendor file was better, then we are in an uncomfortable situation.

I think we have enough experience to conclude here that adding more types of dune files is more confusing than adding new stanzas. Workspace files are quite confusing (but very useful) to users and we learned some lessons from jbuild-ignore.

Marking the feature as experimental initially is relatively cheap in terms of additional development time

Perhaps we should make it even cheaper. I wonder if all we really need is a experimental_stanza function. This would automatically make the stanza available with a suffix: (vendored_dirs.unstable ..). Once we deem it to be production ready, we can simply have the normal stanza definition warn the user that this stanza is no longer experimental and the suffix may be dropped. Otherwise I see making a stanza an extension adds quite a bit of overhead to a fairly compact feature.

[ghost]

I think we have enough experience to conclude here that adding more types of dune files is more confusing than adding new stanzas. Workspace files are quite confusing (but very useful) to users and we learned some lessons from jbuild-ignore.

Yeah, that's a good point. Plus thinking about it more, there is no reason to make a special case for vendored dirs and not other kinds of directories. No objection to release this directly without going through an experimental phase then.

Perhaps we should make it even cheaper. I wonder if all we really need is a experimental_stanza function. This would automatically make the stanza available with a suffix: (vendored_dirs.unstable ..). Once we deem it to be production ready, we can simply have the normal stanza definition warn the user that this stanza is no longer experimental and the suffix may be dropped. Otherwise I see making a stanza an extension adds quite a bit of overhead to a fairly compact feature.

That's an interesting idea. I like it. Thought I'd ague that the second stage should be a hard error, otherwise we risk having leftover .unstable suffixes for stable features, which wouldn't be great.

[NathanReb]

There should be no issue. I think you should set ~allow_approx_merlin based on the status of the current dir.

That works for this particular warning but not for others which can't be disabled as conveniently AFAICT.

Not making it an extension is perfectly fine by me!

[NathanReb]

Ok nevermind the two particular warnings I had in mind or indeed pretty easy to disable in vendored code so I'll just silence them and we can figure out if other warnings need the same treatment.

We should be good after that!

[NathanReb]

And done!

[rgrinberg]

I pushed a commit that cleans up the types a bit and wraps the documentation at 80 chars. LGTM from me. Thanks for this very useful feature.