elixir icon indicating copy to clipboard operation
elixir copied to clipboard

mix clean --deps does not clean out .erl files generated by yecc/leex

Open f355 opened this issue 6 years ago • 11 comments

Current behavior

When running mix clean --deps, the .erl files generated from .xrl/.yrl are left behind.

Expected behavior

All artefacts and intermediate files for the dependencies are deleted after running mix clean --deps, leaving the tree as it looked after mix deps.get, before the compilation.

Steps to reproduce

I'll use Absinthe as an example dependency. I'm using a GitHub tag instead of Hex version, because the artefact pushed to Hex has the .erl files in question included for some reason (which might be a separate issue around mix hex.publish that's outside of the scope for this bug report).

  • Create a new project: mix new mix_bug && cd mix_bug
  • Edit mix.exs to add {:absinthe, git: "https://github.com/absinthe-graphql/absinthe.git", tag: "v1.4.13"} to the list of deps.
  • Fetch the dependencies: mix deps.get
  • Verify there are no .erl files:
$ ls deps/absinthe/src
absinthe_lexer.xrl  absinthe_parser.yrl
  • Compile and clean the project: mix compile && mix clean --deps
  • The .erl files should be gone after cleaning, but they are not:
$ ls deps/absinthe/src/
absinthe_lexer.erl  absinthe_lexer.xrl  absinthe_parser.erl absinthe_parser.yrl

Environment

  • Elixir & Erlang/OTP versions (elixir --version):
$ elixir --version
Erlang/OTP 21 [erts-10.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe] [dtrace]

Elixir 1.7.3 (compiled with Erlang/OTP 21)
  • Mix version:
$ mix --version
Erlang/OTP 21 [erts-10.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe] [dtrace]

Mix 1.7.3 (compiled with Erlang/OTP 21)
  • Operating system: macOS Mojave 10.14

f355 avatar Oct 09 '18 18:10 f355

I was checking the clean task and it uses a simple approach of cleaning the _build/deps folder instead of invoking the deps.clean task like it does for the project. Would make sense to call this task?

If so, I also notice that deps.clean does a similar thing. Would make sense to change deps.clean first to consider those files?

ggcampinho avatar Oct 09 '18 21:10 ggcampinho

@ggcampinho unfortunately I'm not familiar with the codebase enough to definitely answer your questions (assuming they are for me :)), but from a common sense point of view it would be much better to use the same logic for the dependencies as for the project itself.

Also, the dependencies might be built by something other than Mix, e.g. rebar, and probably should be cleaned using that as well to accommodate for custom scripts/hooks, so maybe the cleaning logic should mirror the dependency compilation.

f355 avatar Oct 09 '18 21:10 f355

@ggcampinho yes, I think we need to traverse the deps but we need to be careful with some things:

  1. We can only invoke the mix task for mix dependencies
  2. We need to make sure before invoking the clean task, the dependency is OK, otherwise cleaning can fail or we may even end-up trying to compile the dependency only to clean it

As long as we have those in mind, we should be good. I would start with changing deps.clean and then we can figure out a way of changing clean too.

josevalim avatar Oct 10 '18 08:10 josevalim

I think a simple approach might be to change how the leex and yecc compilers work in Elixir - instead of writing the files to /src and then letting the erlang compiler do the job, we could have them write the temporary .erl file somewhere in /_build and compile to .beam directly. This means we wouldn't have any intermediary files in source directories.

michalmuskala avatar Oct 19 '18 10:10 michalmuskala

I am on this!

josevalim avatar Dec 11 '18 18:12 josevalim

Ok, I am not on this. I think the solution of changing the compiler is not going to fly. 🗡

We probably need to change deps.clean instead, as @ggcampinho first suggested. @ggcampinho are you planning to tackle this?

josevalim avatar Dec 11 '18 19:12 josevalim

Just reading through this thread, I gave it a go on removing those .erl files that get built in deps/{package}/src. This works, but also does not sound like the solution @josevalim outlined here.

We need to make sure before invoking the clean task, the dependency is OK, otherwise cleaning can fail or we may even end-up trying to compile the dependency only to clean it

how do we accomplish making sure the dependency is OK?

Just thought I would try and give this a go!

drincruz avatar Dec 31 '18 13:12 drincruz

@drincruz right. We cannot hardcode the "src" directory because any dependency may have multiple compilers and any other directory can have compiled artefacts. It may be that the best way to solve this is to actually simplify the depending cleaning into two: "build" and "source" and it is always all or nothing, instead of being granular.

josevalim avatar Dec 31 '18 15:12 josevalim

@josevalim yeah, i figured it wouldn't have been as straight forward as i thought!

i think i have a bit of free time before i get busy some again, so i'll try and take a look some more later! 👍

drincruz avatar Dec 31 '18 15:12 drincruz

Another suggestion given by @michalmuskala in #10153 is to emit the .beam files directly from .yrl/.xrl compilers.

josevalim avatar Jul 06 '20 12:07 josevalim

@michalmuskala I looked into your solution and it has complexities too. Namely, the .yrl and .xrl files may depend on other files as behaviours and parse transforms, and we would need to compile those first. It is hard for this to happen in practice but it is technically a limitation (and a backwards incompatible change).

deps.clean may be the best option here after all... it will also be the most correct one for anyone implementing other compilers.

josevalim avatar Jul 14 '20 16:07 josevalim