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

Mix.Task.Compiler.after_compiler callback not executed when build fails #12159

Closed
lukaszsamson opened this issue Sep 29, 2022 · 9 comments
Closed

Comments

@lukaszsamson
Copy link
Contributor

Elixir and Erlang/OTP versions

Erlang/OTP 24 [erts-12.3.2.4] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]
Elixir 1.14.0 (compiled with Erlang/OTP 24)

Operating system

macos

Current behavior

The documentation for Mix.Task.Compiler.after_compiler/2 suggests that the callback will be executed regardless of compilation result. It receives status type as argument. See https://hexdocs.pm/mix/1.14.0/Mix.Task.Compiler.html#after_compiler/2

This simple example demonstrates that the callback is not executed
Repro:

  1. clone https://github.com/lukaszsamson/elixir_after_compile_repro
  2. mix compile
  3. Observe no inspect message
  4. Fix compile error
  5. mix compile
  6. Observe inspect message

Expected behavior

The callback should execute. If that is not possible then it needs to be explicitly documented. IMO there should ba a way of reliably executing cleanup code even if compilation fails.

@josevalim
Copy link
Member

This is... complicated. Should we run the after_compiler for a given compiler even if the compiler does not run at all? Because what happens is that an exception causes later compilers to not run at all. So if :app does not run, should after compilers for it run?

Another option is for boundary to run after the :elixir compiler, reducing the odds it runs too late (or not at all).

/cc @sasa1977

@josevalim
Copy link
Member

And yet another option is to introduce after_compilers, which always runs after any of the compilers, regardless of errors.

@lukaszsamson
Copy link
Contributor Author

It would be best if we could distinguish after self and after all downstream. Are you aware of possible use cases?

@josevalim
Copy link
Member

Right. We could add such feature but I think, for now, hooking after the Elixir compiler would be enough for boundary, especially if :boundary comes immediately before :elixir.

@sasa1977
Copy link
Contributor

Should we run the after_compiler for a given compiler even if the compiler does not run at all?

In the case of boundary, the after_compiler is installed only if the compiler runs. Does that simplify the problem?

Another option is for boundary to run after the :elixir compiler, reducing the odds it runs too late (or not at all).

I'm not sure I follow. Boundary relies on the compilation tracer, and so it needs to install its tracer before the Elixir compiler. After that would be too late, right?

@josevalim
Copy link
Member

What I mean is to set after_compiler(:elixir, …) instead of app. WDYT?

@sasa1977
Copy link
Contributor

Oh, I see. I'm afraid that this would be too early for boundary (at least in its current implementation), because it expects the app to be compiled in the after compiler (e.g. it loads the app modules).

@sasa1977
Copy link
Contributor

Actually this might work. The original issue in elixir-lsp/elixir-ls#717 was caused because boundary doesn't clear its tracer. This is something I could do in the after the Elixir compiler, while the rest would remain as it is today, i.e. execute after app, only on success or noop.

@josevalim
Copy link
Member

Ok. I have updated the docs, so I will close this for now but I will be glad to reopen if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants