Fix fetching optional mix deps by filtering out optionals

[belltoy]

Based on the previous discussions here: https://elixirforum.com/t/optional-mix-deps-being-fetched-with-rebar3-plugins/28417/6

[tsloughter]

Thanks. This is probably all we need, but I want to think about it a bit and maybe @ferd can validate my thinking.

But also, could you add a test?

[ferd]

Thanks for the tests.

The one case I'm sort of concerned with is what do we see with a transitive dependency scenario:

    A
   /  \ 
  B    C 1.2  (optional)
  | 
  C 1.1

We would want to make sure that we actually fetch the right version.

By virtue of being closer to the root, our current algorithm mandates using C1.2, but if it's optional, we may skip it and go for C1.1, and blow that impact up for all transitive dependencies.

It's unclear to me what the proper behaviour would be. Would it make more sense to always fetch the optional ones but not actually build them? Is that chain transitive such that all dependencies of C1.2 also do not get built unless they are depended on by a non-optional dependency?

Unless we have solid answers to these questions, the current pattern of "screw this, we fetch it all" might actually be safer or more sensible for having consistent predictable builds.

[tsloughter]

True. It probably should be fetching 1.2 in such a case :(

[belltoy]

    A
   /  \ 
  B    C 1.2  (optional)
  | 
  C 1.1

In this case, if A is the current project, C 1.2 will always be included. But if it uses == that would be a conflict. Or if it uses ~> that would be ok.

Mix Dependency definition options:

:optional - marks the dependency as optional. In such cases, the current project will always include the optional dependency but any other project that depends on the current project won't be forced to use the optional dependency. However, if the other project includes the optional dependency on its own, the requirements and options specified here will also be applied.

If A is a dependency of other project D:

    D
    |
    A
   /  \ 
  B    C 1.2  (optional)
  | 
  C 1.1

In this case, if use ==, a conflict error must be triggered. Or else, we should follow the B deps and try to find the latest satisfied version, check whether that version satisfies A as well or not. Raise an error if not.

In short, if that would be unable to resolve the dependencies as it's a conflict, we should treat this case as an error.

Rebar3 Dependency Version Handling:

Treating Conflicts as Errors
If you ever want rebar3 to abort as soon as it detects a dependency conflict, instead of skipping the file and proceeding as usual, add the line {deps_error_on_conflict, true}. to your rebar configuration file.

I take a try in mix demo for this case. prometheus_ecto 1.3.0 require ecto ~> 2.0, and the demo project require ecto == 3.0.0 and optional:

  defp deps do
    [
      {:prometheus_ecto, "== 1.3.0"},
      {:ecto, "== 3.0.0", optional: true}
    ]
  end

mix failed:

> mix deps.get
Resolving Hex dependencies...

Failed to use "ecto" (version 3.0.0) because
  prometheus_ecto (version 1.3.0) requires ~> 2.0
  mix.exs specifies == 3.0.0

** (Mix) Hex dependency resolution failed, change the version requirements of your dependencies
or unlock them (by using mix deps.update or mix deps.unlock). If you are unable to resolve the
conflicts you can try overriding with {:dependency, "~> 1.0", override: true}

Optional dependencies in hex specificaitons:

An optional dependency will only be used if a package higher up the dependency chain also depends on it (only if that the dependency is not defined as optional as well).

[belltoy]

BTW, rebar3 hasn't support and/or in version string, e.g ~> 2.0 or ~> 3.0 which would be another issue.

#2364 #2370 #2455 #2486

[tsloughter]

In rebar3 this isn't a conflict. rebar3 doesn't resolve dependencies the way mix does. It simply chooses the version it encounters first in the tree of dependencies, in the example that is 1.2.

[belltoy]

Oh, I see. Rebar3 only fetch and download dependencies in a level-order traversal (breadth-first fashion?), instead of following the strict semantic versioning. Take semver aside, the behaviour of optional of dependencies didn't define. The higher level dependencies will be accepted first, but how about the optional at a higher level?

I think mix does almost the same thing as rebar3 except the strict semver. In detail, mix check match on all deps and detect conflicts, and then reject the conflicts. (ref)

If we need a warning at least if conflicts, this patch will not be enough.

[ferd]

I'm also super doubtful about how good of a behaviour it is to always include the dep at the top level but to drop it if it's one level removed of transitivity. There's no way to ever be able to test that properly since the test has to be over one level removed from the app to work and I flatly don't like how mix does things there.