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

Add 'mix deps.unlock --unused' and `mix deps.clean --unused' #2569

Closed
alco opened this Issue Jul 22, 2014 · 17 comments

Comments

Projects
None yet
6 participants
@alco
Copy link
Member

alco commented Jul 22, 2014

When a dependency is removed from mix.exs, its traces continue to lurk in mix.lock (which will cause it to be fetched on future calls to mix deps.get) and in deps/.

Currently, the only way to remove such a dependency would be to manually call mix deps.unlock <name and mix deps.clean <name>.

It would be more practical to be able to say mix deps.clean --unused --unlock and mix deps.unlock --unused.

@alco

This comment has been minimized.

Copy link
Member Author

alco commented Jul 22, 2014

mix deps shows only used dependencies, so the functionality to filter out unused ones is already there somewhere.

@ericmj

This comment has been minimized.

Copy link
Member

ericmj commented Jul 22, 2014

Should the --all flag unlock/clean unused dependencies?

@alco

This comment has been minimized.

Copy link
Member Author

alco commented Jul 23, 2014

Any reasons for it not to? It makes sense to have --all be a superset of <name> for each dependency.

@nurugger07

This comment has been minimized.

Copy link
Contributor

nurugger07 commented Aug 1, 2014

@ericmj This is similar to our discussion from earlier. I believe mix deps.clean --all should also clean the lock file and mix deps.clean <name> should remove that on dependency. I wouldn't mind taking a look at this issue today if no one is assigned because this has been a nagging issue for me.

@ericmj

This comment has been minimized.

Copy link
Member

ericmj commented Aug 1, 2014

You can pass --unlock if you want unlock dependencies when cleaning. We should not automatically unlock dependencies, it should always be explicit.

@nurugger07

This comment has been minimized.

Copy link
Contributor

nurugger07 commented Aug 1, 2014

@ericmj I would expect clean to clean up any dependency issues, like making sure that the deps in my mix file are the correct version in the lock file. Is the preferred method to call mix deps.clean --all --unlock --unused to remove downloaded deps and remove both unused and used deps from the lock file?

If we are just looking to add an --unused flag to the clean and unlock, I'd still like to take a look at the issue if no one has started it

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Aug 1, 2014

The issue is that deps.clean does not remove a dependency from mix.exs, that's why cleaning should not unlock, because you may clean something simply because it no longer compiles (you want to start from scratch). So you should rather call mix deps.clean --unused --unlock.

If we are just looking to add an --unused flag to the clean and unlock, I'd still like to take a look at the issue if no one has started it

Awesome, go ahead!

@lsimoneau

This comment has been minimized.

Copy link

lsimoneau commented Aug 2, 2014

Shouldn't mix deps.get regenerate the lock file, so deps which have been removed from mix.exs are no longer included?

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Aug 2, 2014

That's actually a good point. In the past we couldn't do that because
deps.get didn't have all dependencies but now it does.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

@ericmj

This comment has been minimized.

Copy link
Member

ericmj commented Aug 2, 2014

Shouldn't mix deps.get regenerate the lock file, so deps which have been removed from mix.exs are no longer included?

No, because we may not always see all dependencies in the mix.exs file. You may have dependencies for only one environment:

def project, do: [deps: deps(Mix.env)]

defp deps(:test), do: [{:some_dep, "~> 1.0"}]
defp deps(_), do: [{:other_dep, "~> 1.0"}]

When you are not on the test environment the dependency would be removed from the mix.lock. It is not recommended to do this, you should use the :only option to filter on environment instead.

Anyway, I still think that unlocking dependencies should be an explicit action.

@ericmj

This comment has been minimized.

Copy link
Member

ericmj commented Aug 2, 2014

Another example is when debugging and you change a dependency to be a path dependency. It is by design that the git/hex lock is kept when you temporarily change the dependency to be a path dep.

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Aug 2, 2014

@ericmj in the path dependency case, the dependency still exists, so nothing will happen. In theory, there is nothing wrong, technically speaking, with pruning the lock file on deps.get. So let's discuss how it affects the user workflow.

If we don't prune on deps.get, the lock file can grow in size but I think this is irrelevant to the issue at hand. The only downside is that someone can add a dependency back in the future and then they are constrained by a lock entry from 6 months ago.

On the other hand, if we automatically prune, we may remove locks in situations we don't really want. For example, when we remove a dependency quickly to try something out or when a dependency is controlled by an external factor, like an environment variable. I have both cases happen with me in ruby projects in the past.

So at first it feels like we are safer by requiring manual pruning. Can anyone thing of other pros and cons?

@wkhere

This comment has been minimized.

Copy link
Contributor

wkhere commented Aug 2, 2014

mix deps.dwim :D

But seriously, please don't go with implicit behaviors. It would feel comfortable at start and then make things crazy.

One compromise is to have some option which turns several of other explicit options.. --unused --unlock seems to be an often case to have an "umbrella" opt for them.

TBH I also think that the semantics of --all accidentally led to some confusion: it does stuff for all deps through all envs, so maybe people started to treat it as --do-what-i-mean. Well I did few times and then I got shot in the foot.

nurugger07 pushed a commit to nurugger07/elixir that referenced this issue Aug 2, 2014

@nurugger07

This comment has been minimized.

Copy link
Contributor

nurugger07 commented Aug 2, 2014

@josevalim @ericmj I pushed my implementation of mix deps.clean --unused to see what you think. I can get the mix deps.unlock --unused wrapped up this evening.

@josevalim

This comment has been minimized.

Copy link
Member

josevalim commented Aug 2, 2014

@nurugger07 I think that's it! We just need to amend the error message here: https://github.com/nurugger07/elixir/blob/187c28527570f24223c3dba7067bd0c6059fc394/lib/mix/lib/mix/tasks/deps.clean.ex#L33 and do a couple style changes (in Elixir's source we don't put spaces after { and before } and remove the space after String.to_atom)!

nurugger07 pushed a commit to nurugger07/elixir that referenced this issue Aug 3, 2014

@ericmj

This comment has been minimized.

Copy link
Member

ericmj commented Aug 19, 2014

@nurugger07 What's the status of this? If you don't have time to complete it please send a PR with what you have so far and we can finish it up.

@nurugger07

This comment has been minimized.

Copy link
Contributor

nurugger07 commented Aug 19, 2014

@ericmj I'll get this wrapped up. It's almost complete now just got a bit swamped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.