Allow specifying why a dep is using override

[josevalim]

I’m suggesting that this is built into the mix dependency resolver. Right now the following happens all the time:

• I want to add foo to my app. I already have bar and baz.
• foo depends on a newer version of bar.
• I can’t update bar because baz depends on it.
• I check how baz and foo use bar, and confirm that its fine to just override.
• So I add {:bar, "~> ...", override: true}
• Someone else adds buzz to the app, which also depends on an old version of bar.
• Problem 1: We never find out that we just overrode bar for the sake of buzz as well.
• Next, foo releases an update that depends on more stuff from the old version of bar.
• Problem 2: mix tells us we can update foo, so we update it and have bugs.

If instead of override: true, I could say:

{:bar, "~> x.x", override: [foo: "x.x.x"]}

which would say “this override only overrides the dependency that foo at exactly version x.x.x has on bar”, then we are protected from any of those accidental changes.

Adding buzz would produce an appropriate dependency conflict warning, solving Problem 1. We can then go look at the code/docs and decide if we want to override the bar dependency for that version of buzz as well.

foo won’t appear to be automatically upgradeable, solving Problem 2.

Originally posted by @zachdaniel in #14080 (comment)

[josevalim]

I believe we should explore this indeed. My suggestion is simply to say:

{:bar, "~> x.x", override: [:foo, :baz]}

Then we can:

• warn if we need to override anyone else
• warn if foo or baz are updated and no longer need the override

I am not sure if override: [dep_name] reads the best, maybe override_for: ..., but using the same option can be a plus.

[whatyouhide]

It doesn't read very well, yeah. If we use :override_for then we will need to do some logic to raise if both are present and everything, but I think maybe us taking that on our shoulders is better for the DX.

[zachdaniel]

I like override_for. I think being able to specify versions in terms of what is being overridden is important, but could be optional. We can also make it nice by accepting version requirements instead of version numbers.

override_for: [foo: "~> 1.0", bar: "~> 1.0"]

If an atom is received, it could just be seen as {:package, ">= 0.0.0"}

Which would also support

override_for: [:foo, bar: "~> 1.0"]

[zachdaniel]

If this is something that seems reasonable to do, I'm happy to take a stab at it. I took a brief look at doing this a few weeks ago, and ultimately the way that override works currently doesn't really help much on this endeavor as its like a "if there are any conflicts, ignore them" check as opposed to happening in the middle of dependency resolution.

[maennchen]

If we're already doing that, I think the items in the override_for list should be full version requirements.

Other common use cases for overrides besides the version are pulling in temporary forks that have a bugfix applied via git.

So something like {:ecto_sql, "~> 3.12", override_for: [{:db_connection, github: "org/project", branch: "my-fix-branch"}]}

I'm however not sure if that is required. This should also be enough:

[
  {:ecto_sql, "~> 3.12", override_for: [:db_connection]},
  {:db_connection, github: "org/project", branch: "my-fix-branch"}
]

[josevalim]

@zachdaniel I don't understand why we need the version in the override_for. You already specify the version that you are using elsewhere in your deps, it seems to be redundant information, and you have no guarantee the requirement of the parent relates to the child (the one overridden) in any way.

@maennchen I believe you have the deps in the example inverted. You'd override db_connection and then you say that you are overriding its usage by ecto_sql.

[zachdaniel]

The idea there is that typically when overriding a dependency you have done some kind of research/checking that it is in fact fine to do so.

My example was bad, using "~> 2.0" is not what you'd do in real life, I think 99% of the time you'd just use ==, but being a version requirement could make things nicer for people who want to be less strict. I'll give an example workflow similar to the one above:

• I want to upgrade package foo, but it depends on bar at ~> 2.0.
• My existing package buz depends on bar at ~> 1.0. buz is currently at version 1.2.0.
• I go through the code of buz and verify that it doesn't use anything that broke from bar's 2.0 release.
• So I add the following dep: {:bar, "~> 2.0", override_for: [buz: "== 1.2.0"]}.

I want to be able to specify that version because there is no guarantee that buz doesn't use some part of the now-broken API from bar in its 1.2.1 release, etc.

If its just override_for: [:buz] then mix deps.update buz will upgrade me to a new version I don't want to be on, even if it still has {:bar, "~> 1.0"}

If I instead just put {:buz, "== 1.2.0"} then I'll never find out if a new version of buz is safe to use with bar ~> 2.0.

If we use my proposal, we get the best of both worlds!

With {:bar, "~> 2.0", override_for: [buz: "== 1.2.0"]}

The mix deps.update buz will get me either

• 1.2.0, if there is no new version that is compatible with {:bar, "~> 2.0"}
• the latest version that is compatible with {:bar, "~> 2.0"} along with a warning letting me know that I no longer need the override.

[josevalim]

I am not sure we can reasonably change the dependency resolution algorithm to consider that. It is already a complex piece of code, with high runtime complexity, and it seems you are introducing conditional resolution.

The resolution should resolve based on the requirements you have explicitly written for buz. Then we do a post-check to check if it still requires an override or not.

[josevalim]

It is also worth adding that it adds complexity to users too. Imagine you are trying mix deps.update buz and nothing happens, and it is hard to understand why if you have conditional resolution happening.

[zachdaniel]

The resolved version would still have to match your explicit requirement of buz. The only thing it is affecting is when the override is considered to apply, which is what we're talking about doing already, right? The override would only work if only that one package is the source of the conflict. This would just be narrowing it further.

Fair enough though. I think it does still leave some room for the class of problems we're trying to alleviate here, but complexity for the user and of the implementation are good reasons. Your call of course :)

We could also add it later, since it would be an extension to just the list of atoms syntax, so the first steps can be the same regardless.

[josevalim]

My proposal is simpler and doesn't require changing the resolution algorithm. If you specify override_for: ..., we do a pass after resolution and make sure that, of all dependencies that depend on the overridden one, only the ones listed in overrides_for actually require the override.