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

Fix fetching optional mix deps by filtering out optionals #2498

Closed
wants to merge 2 commits into from

Conversation

belltoy
Copy link
Contributor

@belltoy belltoy commented Feb 12, 2021

@tsloughter
Copy link
Collaborator

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
Copy link
Collaborator

ferd commented Feb 15, 2021

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
Copy link
Collaborator

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

@belltoy
Copy link
Contributor Author

belltoy commented Feb 15, 2021

    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
Copy link
Contributor Author

belltoy commented Feb 15, 2021

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
Copy link
Collaborator

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
Copy link
Contributor Author

belltoy commented Feb 15, 2021

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
Copy link
Collaborator

ferd commented Feb 15, 2021

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.

@belltoy belltoy closed this Jul 20, 2024
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

Successfully merging this pull request may close these issues.

3 participants