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

Controlling precedence of data_only_dirs vs vendored_dirs #7589

Open
emillon opened this issue Apr 20, 2023 · 4 comments
Open

Controlling precedence of data_only_dirs vs vendored_dirs #7589

emillon opened this issue Apr 20, 2023 · 4 comments

Comments

@emillon
Copy link
Collaborator

emillon commented Apr 20, 2023

  • The current stage of dune about directory status is:

    You can't have data_only_dirs and vendored_dirs at the same time and that is logic.

    > cat dune
    (vendored_dirs subproject)
    (data_only_dirs subproject)
    
    > dune build 
    Error: Directory subproject was marked as data_only and vendored, it can't be
    marked as both.
    

    But you can have (dirs :standard \ subproject) combine with (data_only_dirs subproject) or (vendored_dirs subproject).

    The status (data_only_dirs subproject) or (vendored_dirs subproject) override (dirs :standard \ subproject), why this specification ?

    And what is weird to me is (dirs :standard \ subproject) is equivalent to (data_only_dirs subproject).

  • The need we have is at the moment is:
    A possibility, (dirs :standard \ subproject) override data_only_dirs and/or vendored_dirs instead inverse. This avoids the conflict(ocaml-dockerfile) described below.

    An example: this is a partial tree of solver-service

    > tree solver-service
    solver-service
        |_ dune
        |_ dune-project
        |_ ocaml-dockerfile
            |_ dune-project
        |_ ocaml-ci
            |_ dune
            |_ dune-project
            |_ ocaml-dockerfile
                 |_ dune-project
         
    > cat solver-service/dune
    (vendored_dirs ocaml-dockerfile) ...
    > cat soler-service/ocaml-ci/dune
    (vendored_dirs ocaml-dockerfile) ...
    
    > cd solver-service
    > dune build
    Error: Too many opam files for package "dockerfile-opam":
    - ocaml-ci/ocaml-dockerfile/dockerfile-opam.opam
    - ocaml-dockerfile/dockerfile-opam.opam
    
  • The central point of this PR is to introduce or improve the spec around statuses:

    Getting the prioritization as following (dirs :standard \ subproject) -> (data_only_dirs subproject) -> (vendored_dirs subproject) .

    It is opened to modification, to get the possibility that (dirs :standard \ subproject) override data_only_dirs and/or vendored_dirs without touching the previous relation between data_only_dirs and vendored_dirs.

Originally posted by @moyodiallo in #6577 (comment)

@emillon
Copy link
Collaborator Author

emillon commented Apr 20, 2023

  • You can't have data_only_dirs and vendored_dirs at the same time and that is logic.

I'm not sure about that. data_only_dirs means that the dune file is not interpreted ; vendored_dirs means that certain aliases are not set, and compilation flags are changed. This does not have to be exclusive as @rgrinberg suggested in a previous discussion.
Also, I'm not sure we can set a priority between the options. There might be another use case where the opposite order is required.

The various stanzas that control directories suffer the same issues as the modules fields (#7026) - they have subtle interactions and that's not a great way to expose the underlying model. A better way would be to set the configuration for each directory, but we don't have that at the moment.

To getback to your issue: you're trying to vendor a project that vendors a project, so it creates a conflict because the workspace contains two copies of the dependency.
That's what I would try to fix first. Can you use a released version of ocaml-dockerfile in either of the projects?

If that's not an option, then a dune feature is required to control which configuration wins. One way to do that in the current dune model would be to make a workspace configuration that would control that. (note that workspaces do not compose, so it would not be possible to embed solver-service in another project and have this configuration working).

@moyodiallo
Copy link
Collaborator

I'm not sure about that. data_only_dirs means that the dune file is not interpreted ; vendored_dirs means that certain aliases are not set, and compilation flags are changed. This does not have to be exclusive as @rgrinberg suggested in a previous discussion

At the current implementation the dune file is not interpreted and also mentioned in the docs https://dune.readthedocs.io/en/stable/dune-files.html#data-only-dirs.

To getback to your issue: you're trying to vendor a project that vendors a project, so it creates a conflict because the workspace contains two copies of the dependency.
That's what I would try to fix first. Can you use a released version of ocaml-dockerfile in either of the projects?

We had this in mind, combined with the rate of the dev and correcting bugs it could be painful, this is why we proposed the PR.

@rgrinberg
Copy link
Member

As a general principle, I don't like "overrides" or "priorities" as a user facing feature. We've violated that principle plenty of times already with regrettable results (see the env stanza). If we need to implement something more complicated to avoid such anti-patterns, then so be it. In this particular case, it would mean:

  • Allowing for more than one status per directory (my original suggestion)
  • Allowing multiple packages with the same name to co-exist.

@moyodiallo
Copy link
Collaborator

What about a new status skip_vendored (#6577 (comment)). We allow the status with other statuses for the same directory. The status is going work like, if dirs are vendored we skip them.

(skip_vendored dirs)

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