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

Improve handling of local dependencies #401

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

josevalim
Copy link
Contributor

Treat all path dependencies as files in the current
project instead of remote dependencies. This simplifies
the logic as we no longer need to especially handle
umbrella applications, as they are all path dependencies
anyway.

This requires Elixir v1.10+.

Also note this patch completely removes :plt_add_deps
because it did not work before. Since the Plt.check/2
function always adds all transitive dependencies, any
attempt to discard deps by setting :plt_add_deps to
:apps_deps had no effect. This PR does not remove
:apps_deps from the documentation but I would
recommend so to be done.

Treat all path dependencies as files in the current
project instead of remote dependencies. This simplifies
the logic as we no longer need to especially handle
umbrella applications, as they are all path dependencies
anyway.

This requires Elixir v1.10+.

Also note this patch completely removes `:plt_add_deps`
because it did not work before. Since the `Plt.check/2`
function always adds all transitive dependencies, any
attempt to discard deps by setting `:plt_add_deps` to
`:apps_deps` had no effect. This PR does not remove
`:apps_deps` from the documentation but I would
recommend so to be done in a separate PR.
@josevalim
Copy link
Contributor Author

Note this PR has one issue. If A depends on B as a path dependency, both B and A are verified when running mix dialyzer from A. That's how Mix treats path dependencies (as if they were part of the same project), but it now means you may get warnings from B directly in A.

There is one obvious solution to this problem, which is to automatically ignore all warnings from B when running the command from A, but I haven't implemented that because I am not sure if it is desired or not.

@jeremyjh
Copy link
Owner

Note this PR has one issue. If A depends on B as a path dependency, both B and A are verified when running mix dialyzer from A. That's how Mix treats path dependencies (as if they were part of the same project), but it now means you may get warnings from B directly in A.

I believe that is how it has always worked because we'd actually run from the root in an Umbrella project, though it has been a requested change in the past to limit warnings to the current child. This PR makes that much more feasible but we can add an issue for it rather than address it in this PR.

On another note, if this requires Elixir 1.10 should we add elixir: "elixir: ">= 1.10.0" in the project key? I'm not totally clear if this is used by hex in version resolution.

@josevalim
Copy link
Contributor Author

On another note, if this requires Elixir 1.10 should we add elixir: "elixir: ">= 1.10.0" in the project key? I'm not totally clear if this is used by hex in version resolution.

We should. It is not used by Hex but you should update it. I didn't do the change because it most likely requires changing other places too, such as CI configs and so on. :) Perhaps we should do the change on master first before merging this PR?

@jeremyjh
Copy link
Owner

Ok, I'll do one more release from master and then update configs and README for a new minimum Elixir version before merging this.

@Nezteb
Copy link
Sponsor Contributor

Nezteb commented May 2, 2023

FYI I pulled this and did a rebase of upstream master and everything seems to still work as expected. 😄

@jeremyjh
Copy link
Owner

jeremyjh commented May 2, 2023

Yeah, I wish I'd documented this at the time but I ran into a doubt regarding these changes; from what I recall I found that plt_add_deps did work, but I'm not sure I'd proved that to my satisfaction and hence didn't object to it, but wasn't ready to merge it either. I meant to take another look at it, and then three years passed. Thanks for putting it back on my radar, but all I can really say is I have to give another serious look.

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.

None yet

3 participants