Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vendored_dirs and release mode #3911

Open
NathanReb opened this issue Nov 3, 2020 · 6 comments
Open

Vendored_dirs and release mode #3911

NathanReb opened this issue Nov 3, 2020 · 6 comments
Assignees

Comments

@NathanReb
Copy link
Collaborator

Related to #3909.

At the moment when building a package in release mode (dune build -p <package>) dune is allowed to build its dependencies that live in directories marked as vendored_dirs even if they belong to a different package. This behaviour fits well when one uses vendoring to bundle dependencies of an executable they are releasing.

It happens that some might want to use vendoring for other purposes than bundling dependencies. For example if they want to work with their dependencies embedded to avoid depending on opam for development. They might also want to modify some of their dependencies during development and wait for the changes to be upstreamed before the release. In both cases they would use local vendored copies for development but would still rely on opam to manage those dependencies once they release.

In those cases, the vendored dependencies are declared as dependencies in the opam files, meaning that when installing them, the default build command dune build -p <package> might fail because of a conflict between an opam installed dependency and its local vendored copy.

I worked on a minimal example of this behaviour in this repository: https://github.com/samoht/vendors-test
All the steps necessary to reproduce are detailed in the README.

Ultimately when trying to install one of the packages you get the following error:

Error: Conflict between the following libraries:
- "c" in _build/default/vendors/c
- "c" in .../_opam/lib/c
  -> required by library "b" in .../_opam/lib/b

There are ways around this problem at the moment, for example to remove the vendored directories from the release tarball or to remove it as part of the build instructions in the opam file.

My main concern here is that the default behaviour of dune might be a bit counter intuitive and results in failures that in some cases might not be visible straight away.

I'm not sure what the vendoring behaviour should be in this case with the new design but we should at least document this and the existing workarounds.

@rgrinberg rgrinberg self-assigned this Nov 11, 2020
@rgrinberg
Copy link
Member

rgrinberg commented Nov 12, 2020

It happens that some might want to use vendoring for other purposes than bundling dependencies. For example if they want to work with their dependencies embedded to avoid depending on opam for development. They might also want to modify some of their dependencies during development and wait for the changes to be upstreamed before the release. In both cases they would use local vendored copies for development but would still rely on opam to manage those dependencies once they release

I propose that we drop the term vendoring to describe this use case. This is much more similar to traditional package management, and It has quite different requirements from what is traditionally meant by vendoring. If we try to fit both of these use cases under one roof, we'll likely find that our solution satisfies neither camp.

To give an example of what I mean, here's an instance of both vendoring and package management in ocamllsp:

The lsp package vendors stdune, but includes uutf library as a submodule for local development. On the other hand, ocaml-lsp-server vendors the packages lsp, uutf, and merlin.

Here's a little summary of the kind of behavior I might expect from either feature:

Property Vendoring Package Management
Hiding Warnings Yes Yes
Name Mangling Yes No
Release vs. Development Vendored sources remain constant Fetched by package manager depending on dev/release
dune clean Cleans vendored artifacts Should not clean packages
Nesting Vendored packages may have their own vendored packages Packages are fetched into a flat namespace

For the package management case you have in mind, it sounds like the simplest solution is to add a way to use additional package sources in development. Something like this for example:

(enabled_if (= %{profile} dev)
 (data_only_dirs :standard _sources)) ;; _sources is otherwise ignored

Perhaps, we could consider a higher level construct that would ignore warnings for free for us. Nevertheless, the basic idea remains. Of course, the implementation of this greatly complicated by dune supporting building multiple profiles at once, but that's a tractable problem.

@NathanReb
Copy link
Collaborator Author

One thing I would add is formatting, we don't want formatting enabled in any of those. Not sure whether it should fall into the same category as warnings but I do feel like there is a common pattern here, i.e. it's code we don't want to modify ourselves so maybe it makes to have a feature to disable both formatting and warnings at the same time.

@nojb
Copy link
Collaborator

nojb commented Nov 12, 2020

One thing I would add is formatting, we don't want formatting enabled in any of those. Not sure whether it should fall into the same category as warnings but I do feel like there is a common pattern here, i.e. it's code we don't want to modify ourselves so maybe it makes to have a feature to disable both formatting and warnings at the same time.

I recently ran into this problem. Instead of bundling up everything into one concept, why not extend the formatting stanza to be able to enable/disable automatic formatting on a per-directory basis, eg we could write

(formatting (dirs :standard \ foo))

to disable formatting in a directory foo (and all its subdirectories).

@NathanReb
Copy link
Collaborator Author

That'd be fine by me but if we split everything into separate features it then becomes quite tedious for users to set all those things everytime they vendor something. Maybe there's value into cutting this into several features and just add shortcuts for usecases that need all of them at once!

That being said, I'd be all for moving the formatting exclusion mechanism from ocamlformat to dune as it has other use cases and having all those different files to configure it all when dune is ultimately responsible of formatting files is a bit annoying.

@nojb
Copy link
Collaborator

nojb commented Nov 12, 2020

That'd be fine by me but if we split everything into separate features it then becomes quite tedious for users to set all those things everytime they vendor something. Maybe there's value into cutting this into several features and just add shortcuts for usecases that need all of them at once!

But in this case, the formatting stanza already exists and enabling/disabling formatting for a subdirectory makes sense independently of vendoring, so I still think this would be a good idea. And it would not be subsumed by .ocamlformat-ignore because it can also affect dune formatting.

Eventually, the vendoring mechanism can set some good defaults, but having independent control via formatting would still be valuable.

@rgrinberg
Copy link
Member

One thing I would add is formatting, we don't want formatting enabled in any of those. Not sure whether it should fall into the same category as warnings but I do feel like there is a common pattern here, i.e. it's code we don't want to modify ourselves so maybe it makes to have a feature to disable both formatting and warnings at the same time.

Yeah, I forgot about this one.

That'd be fine by me but if we split everything into separate features it then becomes quite tedious for users to set all those things everytime they vendor something. Maybe there's value into cutting this into several features and just add shortcuts for usecases that need all of them at once!

That sounds good to me. I think the way to think about it is how we handle -p. It's just a collection of separate properties that is grouped into a single option.

Not sure whether it should fall into the same category as warnings but I do feel like there is a common pattern here, i.e. it's code we don't want to modify ourselves so maybe it makes to have a feature to disable both formatting and warnings at the same time.

Perhaps, but I think that these features are still useful on their own. For example, it's useful to be able to disable formatting on generated files. You don't necessarily want to disable warnings on these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants