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

Elixir 1.15 #512

Merged
merged 5 commits into from
Aug 26, 2023
Merged

Elixir 1.15 #512

merged 5 commits into from
Aug 26, 2023

Conversation

danschultzer
Copy link
Contributor

Resolves #511

This uses Mix.ensure_application!/1 instead of Code.ensure_loaded?/1. This is similar to how missing erlang libraries are dealt with in other repos:

nccgroup/sobelow#143
phoenixframework/tailwind@68185c7

This PR also tests on Elixir 1.15 and OTP 26, but there are issues getting the tests to succeed. I'll see if I can get the tests to succeed.

@danschultzer
Copy link
Contributor Author

@jeremyjh it would be helpful to me if you can approve the workflow so the CI can run. I have to switch in local right now to test between them.

@danschultzer danschultzer force-pushed the elixir-1.15 branch 2 times, most recently from a3d7da8 to 0fa00da Compare July 19, 2023 21:24
@jeremyjh
Copy link
Owner

I changed the workflow settings so it shouldn't prompt again.

@jonnystorm
Copy link

@danschultzer FYI: I just got around to engaging those errors for my own project. For what it's worth, I cleaned up tests with this commit. Some additions are reasonable while others are clearly kludges, but none of the added requires, etc. are superfluous (that I can see), and none can be pushed down deeper into the dependency tree.

I plan to take a similar inventory of the various errors observed while running as an archive with mix, but feel free to take whatever is useful from the above and throw away the rest.

@danschultzer danschultzer force-pushed the elixir-1.15 branch 4 times, most recently from 09fd7a2 to 0ea8535 Compare July 23, 2023 00:01
@danschultzer danschultzer marked this pull request as ready for review July 23, 2023 00:07
@danschultzer
Copy link
Contributor Author

danschultzer commented Jul 23, 2023

Thanks @jonnystorm! I noticed that the dialyxir load path disappears with certain test cases using :code.get_path, and this only happens after the Mix.Project.in_project calls. So I've set prune_code_paths: false on all fixtures.

This may break in the future and I would rather ensure that the Mix.Project.in_project callback doesn't cause the dialyxir path to be unloaded. I couldn't grok how to fix that. I guess all fixtures has to be updated to load the dialyxir dependency, but I couldn't get it to work properly.

@jeremyjh this should be ready for review!

@elishagreenwald
Copy link

Hi is there a plan to merge this soon? Wondering if I should explore a workaround / hold off on upgrading etc... any timelines would be super helpful. Thanks for all the great work folks!

lib/mix/tasks/dialyzer.ex Outdated Show resolved Hide resolved
if Version.match?(System.version(), ">= 1.15.0") do
defp check_dialyzer do
if function_exported?(Mix, :ensure_application!, 1) do
Mix.ensure_application!(:dialyzer)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this raises - for example because they installed erlang on debian without dialyzer - is it obvious what the user needs to do next? That is why we are printing the extended error message in the other branch of this top-level if. I think we should trap this and print the message in this case as well, maybe just move the message itself to separate function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the message printed: https://github.com/elixir-lang/elixir/blob/a94700f861706482f6fe7f5abb884f235763db6f/lib/mix/lib/mix.ex#L622-L627

Which would read as:

The application "dialyzer" could not be found. This may happen if your
Operating System broke Erlang into multiple packages and may be fixed
by installing the missing "erlang-dev" and "erlang-dialyzer" packages

It should be clear enough right?

@jeremyjh
Copy link
Owner

@danschultzer @elishagreenwald Hi guys, sorry to leave this languishing, I'd somehow missed it was complete. Been super busy lately. I have just a couple of small asks to change before we merge.

[app: :local_plt_no_warn, version: "1.0.0", dialyzer: [plt_file: {:no_warn, "local.plt"}]]
[
app: :local_plt_no_warn,
prune_code_paths: false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prune_cod_paths is a workaround for apps that aren't specifying all their runtime dependencies. Could add dialyzer to extra_applications instead? I don't think prune_code_paths is what we want to recommend people do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's Dialyxir that gets pruned, and I haven't been able to find a way to prevent Elixir from pruning it. Halfway through the tests all the Dialixir modules are gone. I've tried extra_applications, and setting it as a dep. Only thing I've found to work is to setting that flag.

This could break in the future, so it would be nice to find a better solution, but I haven't figured out why Mix.Project.in_project would cause the dialixir code paths to be pruned in the first place.

@jeremyjh jeremyjh merged commit 997e5bc into jeremyjh:master Aug 26, 2023
12 checks passed
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.

mix dialyzer reports dependency missing
4 participants