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 clean --deps does not clean out .erl files generated by yecc/leex #8262

Open
f355 opened this issue Oct 9, 2018 · 11 comments
Open

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

f355 opened this issue Oct 9, 2018 · 11 comments

Comments

@f355
Copy link

f355 commented Oct 9, 2018

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
@ggcampinho
Copy link
Contributor

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?

@f355
Copy link
Author

f355 commented Oct 9, 2018

@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.

@josevalim
Copy link
Member

@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.

@michalmuskala
Copy link
Member

michalmuskala commented Oct 19, 2018

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.

@josevalim josevalim added this to the v1.8.0 milestone Dec 7, 2018
@josevalim
Copy link
Member

I am on this!

@josevalim
Copy link
Member

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 josevalim removed this from the v1.8.0 milestone Dec 11, 2018
@drincruz
Copy link
Contributor

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!

@josevalim
Copy link
Member

@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.

@drincruz
Copy link
Contributor

@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! 👍

vgsantoniazzi added a commit to vgsantoniazzi/elixir that referenced this issue Jul 4, 2020
When cleaning with --deps flag, iterate over dependencies and remove generated .erl files.

This feature was discussed in elixir-lang#8262.
@josevalim
Copy link
Member

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

@josevalim
Copy link
Member

@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.

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

No branches or pull requests

5 participants