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

Proposal: "Dependency Groups" or "one of these dependencies is required, w/ a default" #899

Closed
zachdaniel opened this issue Aug 13, 2021 · 7 comments

Comments

@zachdaniel
Copy link

zachdaniel commented Aug 13, 2021

The problem

Ash Framework comes with a dependency called picosat_elixir, which uses a NIF. This causes problems for some users, so I've implemented a (slower) alternative natively in Elixir using csp. However, I don't really want most people using the csp dependency unless they encounter problems with the picosat_elixir dependency. So I'm looking for behavior that is essentially "one of these dependencies needs to be specified, but if none of them are, use picosat_elixir as the default". This seems like it could be useful for other use cases, like json_formatter or http_library, allowing for a more seamless initial installation process, and automatically choosing the recommended library.

The temporary hacky workaround

I haven't actually released this yet, but in Ash we'd have:

{:picosat_elixir, "~> 0.1.5"},
{:csp, "~> 0.1.0", optional: true},

And then there is a troubleshooting guide that instructs people who really can't get picosat_elixir set up to add the following to their dependencies:

{:picosat_elixir, override: true, only: []},
{:csp, "~> 0.1.0"},

Less than ideal :D

The proposal: Optional Dependency Groups

This is just one potential way it could be implemented:

defp deps do
  {:picosat_elixir, "~> 0.1.5", group_default: true, group: "Sat Solver"},
  {:csp, "~> 0.1.0", group: "Sat Solver"}
end

We could also use priority, e.g group_priority: 1, group_priority: 2, and then choose the highest priority dependency that has no conflicts.

And then of course if one of the dependencies is explicitly configured in the user's deps then we just use that one no matter what.

The benefits of a strategy like this:

Right now, the typical way of switching on dependencies is something like:

if Code.ensure_loaded?(ModuleInDependency) do
  ...
end

That is theoretically brittle depending on what module you choose. Some other dependency they use could also (very unlikely, but possible) define a module of the same name. But since mix has the group names, we could do something like this:

if Mix.group_choice(:ash, "Sat Solver") == :picosat_elixir do
  ....
end

I'm sure there are probably a few aspects of this I haven't considered, but it seems like it could be useful for others as well.

We could also display the group something was installed for/the reason of it when installing, e.g

csp 0.1.0 (Group: "Sat Solver")
@Eiji7
Copy link

Eiji7 commented Aug 13, 2021

Example case from archived library:
https://github.com/unnawut/licensir/tree/master/lib/table_rex

image

This shows that sometimes checking if specific module is loaded is not enough …

@wojtekmach
Copy link
Member

The temporary hacky workaround

My recommendation would be to in your library define both deps as optional:

{:picosat_elixir, "~> 0.1.5", optional: true},
{:csp, "~> 0.1.0", optional: true},

and then pick whichever happens to be available, possibly in a given order, and when neither is available you can give a really good error message. Here's one: https://github.com/swoosh/swoosh/blob/v1.5.0/lib/swoosh/api_client/hackney.ex#L13:L30. You could even make solver backend explicitly configurable.

My initial impression is deps groups is perhaps a little bit too specific. If there's to be a new mechanism, I'd be interested in one that can be used for a broader set of use cases. Btw, is this Hex specific or we should have it for Git deps too? If so, it's not a Hex discussion but a Mix one. :)

All that being said, I think this is definitely a problem worth looking at so I'm super curious how the discussion goes.

@zachdaniel
Copy link
Author

Ah, you're totally right about it being a mix discussion, sorry :). And yeah, if this discussion turned out not to yield anything I was planning on going that route, but in terms of optimizing for user experience, not having to specify when you don't care what is used is a slightly better experience (IMO).

Do we have the capability to transfer issues from here to there? Or should I just copy it and everyone's comments over to a new issue?

@wojtekmach
Copy link
Member

wojtekmach commented Aug 13, 2021

We tend not to discuss proposals in the Elixir issue tracker, start a ML thread :)

@wojtekmach
Copy link
Member

wojtekmach commented Aug 13, 2021

but in terms of optimizing for user experience, not having to specify when you don't care what is used is a slightly better experience (IMO).

Oh, given you mentioned picosat has NIFs, my guess would be that the problem is not deps conflicts and such but rather the dependency can be resolved but cannot be compiled. Because C compilers, Windows, etc. So I believe in this particular case your proposal would not help, I don't think there should be automatic system to pick dependency B if dependency A failed to compile. Deps resolution and compilation are separate steps and I think it would be complicated to change that.

@zachdaniel
Copy link
Author

Yes, you are correct, but they'd only need to go looking for a solution if they had that problem, which there would be a guide for solving in the docs. Its just that I don't really want people choosing csp unless its a last resort. I don't want it looking like a viable option. But if nothing comes of this I'll probably go the explicit route since it is more idiomatic, and the docs will say "please don't use csp unless you must", that kind of thing.

@zachdaniel
Copy link
Author

I also then have to include in all of the various extension packages that same optional dependency so that they can compile to test in CI, which isn't too big of a deal, but it is slightly annoying :D

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