-
Notifications
You must be signed in to change notification settings - Fork 517
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
Conversation
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? |
Thanks for the tests. The one case I'm sort of concerned with is what do we see with a transitive dependency scenario:
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. |
True. It probably should be fetching 1.2 in such a case :( |
In this case, if A is the current project, C 1.2 will always be included. But if it uses Mix Dependency definition options:
If A is a dependency of other project D:
In this case, if use 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:
I take a try in mix demo for this case. 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:
|
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. |
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 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. |
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. |
Based on the previous discussions here: https://elixirforum.com/t/optional-mix-deps-being-fetched-with-rebar3-plugins/28417/6