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

Do not load optional applications #509

Closed
wants to merge 1 commit into from
Closed

Conversation

aptinio
Copy link

@aptinio aptinio commented Jul 6, 2023

@josevalim
Copy link
Contributor

Unfortunately this is not correct. You don't want to delete all optional applications, you still want to load them. Instead, you want to not report them in case they cannot load.

@aptinio
Copy link
Author

aptinio commented Jul 6, 2023

@josevalim, you got me thinking... so I tested it out to make sure. The computed deps_apps in the project I'm working on are the same on Elixir 1.14 without this PR and on Elixir 1.15 with this PR.

Since deps_apps/2 is being called recursively, the optional deps that are explicitly installed still get included in the first call to deps_apps.

Also, as far as I can tell, this PR is replicating the old behavior that you mentioned in https://elixirforum.com/t/elixir-v1-15-0-rc-0-released/56019/27:

In earlier Elixir versions, foo.app would look like this if gettext was not added as dependency somewhere:

{applications, []}

But now it looks like this:

{optional_applications, [gettext]}
{applications, [gettext]}

What do you think? Thanks for looking into this! ☺️

@josevalim
Copy link
Contributor

This will be true because you don't use gettext. But someone may use gettext, and now it will be removed, while it should have been listed.

@GRoguelon
Copy link
Contributor

@josevalim I made a proposal on @aptinio 's repo: aptinio#1

@GRoguelon
Copy link
Contributor

Hi @jeremyjh,

I've submitted a PR for the issue related to Unknown application with optional applications. Should I create a new PR directly in this repo?

Thank you.

@jeremyjh
Copy link
Owner

@GRoguelon yes please make a new PR on this repo and we'll close this one.

@GRoguelon
Copy link
Contributor

@jeremyjh Done: #514

@aptinio aptinio deleted the fix-#502 branch August 26, 2023 22:41
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.

Dialyxir attempts to load transitive optional dependencies
4 participants