Add a :make compiler #4134

Closed
wants to merge 5 commits into
from

Projects

None yet

6 participants

@whatyouhide
Member

I'm opening this PR to test the water regarding the make compiler mentioned by #4082. It doesn't support nmake and it's pretty barebones, but it could be a starting point. Here, just a couple of options are configurable: :make_cwd to choose the cwd for running make, and :make_targets for the list of make targets to be run. We could make other options configurable, like specifying a Makefile. Also, right now everything that make outputs is streamed to stdout, we could make that configurable as well (e.g., :make_report_only_errors or something similar).

Wdyt?

@josevalim
Member

It looks like a good point starting point. Don't forget to call build_structure too. There is a very detailed set of outcomes in comeonin too: https://github.com/elixircnx/comeonin/blob/master/mix.exs

@whatyouhide
Member

@josevalim thanks for the pointer to comeonin, lots of good strategy in its mixfile :). I have a design question: we could either provide an option like :make_makefile to choose the makefile to use or force it to be Makefile (and Makefile.win on windows). If we go with the first option, we'd still take care of /F to pass to nmake so that users can do something like this:

[...,
 make_makefile: (if match?({:win32, _}, :os.type()), do: "Makefile.win", else: "Makefile")]

and we'd run nmake /F Makefile.win or make respectively.

Wdyt?

@josevalim
Member

I think we could do both. The question is, should we support non-conventional makefiles for make too?

@josevalim
Member

Also, the target also changes depending on the platform. :(

@whatyouhide
Member

Yes the target changes, but users could rely (not every time but most of the time of course) on the fact that make/nmake run the first target in the makefile when called with no arguments. People can still do the :os.type/1 dance to set the value of :make_targets based on the platform.

@whatyouhide
Member

This is ready for review. I added the following options:

  • :make_makefile - used to specify the Makefile to use (takes care of -f//F based on the OS)
  • :make_error_message - used to specify a custom error message in case make fails.

:make_error_message was inspired by how many infos comeonin includes when it fails (stuff like "install gcc" or "if you're on Windows, install Visual Studio ...". Since we cannot predict the requirements of a library so that it compiles, I figured letting the library author provide the error message may be the easiest way. Let me know what you think!

(PS sorry it took me so long to work again on this)

@josevalim josevalim and 1 other commented on an outdated diff Jan 16, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ config = Mix.Project.config()
+ build(config)
+ Mix.Project.build_structure
+ :ok
+ end
+
+ defp build(config) do
+ makefile = Keyword.get(config, :make_makefile, :default)
+ targets = Keyword.get(config, :make_targets, [])
+ cwd = Keyword.get(config, :make_cwd, ".")
+ error_msg = Keyword.get(config, :make_error_message, "")
+ exec = executable_for_current_os()
+
+ args = args_for_makefile(exec, makefile) ++ targets
+
+ exit_status = File.cd!(cwd, fn -> cmd(exec, args) end)
@josevalim
josevalim Jan 16, 2016 Member

You can pass a cd option to System.cmd

@whatyouhide
whatyouhide Jan 16, 2016 Member

Ah, awesome :) Fixed in the next commit!

@whatyouhide whatyouhide changed the title from Add the first (draft-y) version of a Make compiler to Add a :make compiler Jan 16, 2016
@josevalim josevalim commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ * `:make_error_message` - it's a binary. It's a custom error message that
+ can be used to give instructions as of how to fix the error (e.g., it can
+ be used to suggest installing `gcc` if you're compiling a C dependency).
+
+ """
+
+ @spec run([binary]) :: :ok | no_return
+ def run(_args) do
+ config = Mix.Project.config()
+ build(config)
+ Mix.Project.build_structure
+ :ok
+ end
+
+ defp build(config) do
+ makefile = Keyword.get(config, :make_makefile, :default)
@josevalim
josevalim Jan 20, 2016 Member

Should we also expose make_executable? The default is the OS lookup we perform. :)

@josevalim
Member

This looks really good, I have added just one last comment. :)

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ def run(_args) do
+ config = Mix.Project.config()
+ build(config)
+ Mix.Project.build_structure
+ :ok
+ end
+
+ defp build(config) do
+ makefile = Keyword.get(config, :make_makefile, :default)
+ targets = Keyword.get(config, :make_targets, [])
+ cwd = Keyword.get(config, :make_cwd, ".")
+ error_msg = Keyword.get(config, :make_error_message, "")
+ exec = executable_for_current_os()
+
+ args = args_for_makefile(exec, makefile) ++ targets
+ exit_status = cmd(exec, args, cwd)
@lexmag
lexmag Jan 20, 2016 Member

It'd be cleaner if we case on cmd/3 right away:

case cmd(exec, args, cwd) do
  0 -> :ok
  exit_status ->
    build_error(exec, exit_status, error_msg)
end
@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ if exit_status == 0, do: :ok, else: build_error(exec, exit_status, error_msg)
+ end
+
+ # Runs `exec [args]` in `cwd` and prints the stdout and stderr in real time,
+ # as soon as `exec` prints them (using `IO.Stream`).
+ defp cmd(exec, args, cwd) do
+ opts = [into: IO.stream(:stdio, :line),
+ stderr_to_stdout: true,
+ cd: cwd]
+
+ {%IO.Stream{}, status} = System.cmd(executable(exec), args, opts)
+ status
+ end
+
+ defp executable(exec) do
+ if found = System.find_executable(exec) do
@lexmag
lexmag Jan 20, 2016 Member

It is the same as System.find_executable(exec) || executable_not_found_error(exec).

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+
+ {%IO.Stream{}, status} = System.cmd(executable(exec), args, opts)
+ status
+ end
+
+ defp executable(exec) do
+ if found = System.find_executable(exec) do
+ found
+ else
+ executable_not_found_error(exec)
+ end
+ end
+
+ defp executable_not_found_error(exec) do
+ Mix.raise """
+ `#{exec}` not found in the current path.
@lexmag
lexmag Jan 20, 2016 Member

Mix error message shouldn't have trailing punctuation.

@lexmag lexmag and 1 other commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ else
+ executable_not_found_error(exec)
+ end
+ end
+
+ defp executable_not_found_error(exec) do
+ Mix.raise """
+ `#{exec}` not found in the current path.
+ """
+ end
+
+ defp build_error(exec, exit_status, error_msg) do
+ msg = """
+ Could not compile with `#{exec}`.
+ """
+ Mix.raise(Enum.join([msg, error_msg], "\n"))
@lexmag
lexmag Jan 20, 2016 Member

Enum.join feels more than we need for concatenation of two strings. :bowtie:

@whatyouhide
whatyouhide Jan 20, 2016 Member

I don't want to take care of the newline if either msg or error_msg are missing, that's why Enum.join/2 - "\n" will only be there if they're both there :)

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ defp executable(exec) do
+ if found = System.find_executable(exec) do
+ found
+ else
+ executable_not_found_error(exec)
+ end
+ end
+
+ defp executable_not_found_error(exec) do
+ Mix.raise """
+ `#{exec}` not found in the current path.
+ """
+ end
+
+ defp build_error(exec, exit_status, error_msg) do
+ msg = """
@lexmag
lexmag Jan 20, 2016 Member

I think string definition would be better than heredoc for single line.

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ cd: cwd]
+
+ {%IO.Stream{}, status} = System.cmd(executable(exec), args, opts)
+ status
+ end
+
+ defp executable(exec) do
+ if found = System.find_executable(exec) do
+ found
+ else
+ executable_not_found_error(exec)
+ end
+ end
+
+ defp executable_not_found_error(exec) do
+ Mix.raise """
@lexmag
lexmag Jan 20, 2016 Member

I think string definition would be better than heredoc for single line.

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ `"Makefile"` for Unix systems and `"Makefile.win"` for Windows systems.
+
+ * `:make_targets` - it's a list of binaries. It's the list of Make targets
+ that should be run. Defaults to `[]`, meaning `make` will run the first
+ target.
+
+ * `:make_cwd` - it's a binary. It's the directory where `make` will be run,
+ relative to the root of the project.
+
+ * `:make_error_message` - it's a binary. It's a custom error message that
+ can be used to give instructions as of how to fix the error (e.g., it can
+ be used to suggest installing `gcc` if you're compiling a C dependency).
+
+ """
+
+ @spec run([binary]) :: :ok | no_return
@lexmag
lexmag Jan 20, 2016 Member

[binary] –> OptionParser.argv

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/lib/mix/tasks/compile.make.ex
+ stderr_to_stdout: true,
+ cd: cwd]
+
+ {%IO.Stream{}, status} = System.cmd(executable(exec), args, opts)
+ status
+ end
+
+ defp executable(exec) do
+ if found = System.find_executable(exec) do
+ found
+ else
+ executable_not_found_error(exec)
+ end
+ end
+
+ defp executable_not_found_error(exec) do
@lexmag
lexmag Jan 20, 2016 Member

Maybe raise_executable_not_found? Feels more actionable and reveal what's happening inside.

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/test/mix/tasks/compile.make_test.exs
+ in_fixture "compile_make", fn ->
+ File.write! "Makefile", """
+ my_target:
+ \t@echo "hello"
+ """
+
+ assert capture_io(fn -> run() end) =~ "hello\n"
+ end
+ end
+
+ test "specifying targets" do
+ in_fixture "compile_make", fn ->
+ File.write! "Makefile", """
+ useless_target:
+ \t@echo "nope"
+ my_target:
@lexmag
lexmag Jan 20, 2016 Member

Maybe we could drop my_ prefix? :bowtie:

@lexmag lexmag commented on an outdated diff Jan 20, 2016
lib/mix/test/mix/tasks/compile.make_test.exs
@@ -0,0 +1,99 @@
+Code.require_file "../../test_helper.exs", __DIR__
+
+defmodule Mix.Tasks.Compile.MakeTest do
+ use MixTest.Case
+ import ExUnit.CaptureIO
+
+ setup do
+ Mix.Project.push MixTest.Case.Sample
+ :ok
+ end
+
+ test "running without a makefile" do
+ msg = ~r/^Could not compile with/
@lexmag
lexmag Jan 20, 2016 Member

Shall we use \A real anchor for the beginning of the string to be precise?

@whatyouhide
Member

Hey @lexmag and @josevalim, I should have fixed all the stuff both of you mentioned (and added the :make_executable option). Let me know how it looks! :)

@lexmag
Member
lexmag commented Jan 20, 2016

Wonderful. 💛

@lexmag
Member
lexmag commented Jan 26, 2016

Looks like Travis makes tests to fail, we need to find a way to skip "Entering directory" and "Leaving directory" output.

@whatyouhide
Member

@lexmag oops, you're right. What if we check the captured io with =~ instead of ==? Too liberal?

@tuvistavie
Contributor

Looks like Travis makes tests to fail, we need to find a way to skip "Entering directory" and "Leaving directory" output.

Maybe it would be nice if there were a :make_options key to which we could pass options we want to pass to make. I think it would be useful enough, and passing the -s (or --no-print-directory) option here would fix the issue.

@whatyouhide
Member

Sorry for the long silence period! @tuvistavie I guess we could have :make_options, but at that point we could use that for :make_targets and :make_makefile as well :\ Thoughts?

@riverrun
Contributor

A few comments / questions:

  1. Has this been tested with mix deps.compile as well as mix compile? I've been caught out in the past by slight differences in how these two commands work.
  2. With comeonin, there were on_load errors every time Erlang or Elixir was updated (this had something to do with the version of the compiled file not matching the runtime version). My solution was to remove the priv directory every time it's compiled, so as to force the C code to be rebuilt.
  3. There's a discussion about making the build process easier for Windows users here.
@josevalim
Member

Thank you @riverrun. I will verify 1. 2 can be fixed by adding a manifest. Which particular point about 3 do you think we should incorporate?

@riverrun
Contributor

About number 3, that's still under discussion. I'll get back to you on this thread soon.

@riverrun
Contributor

So, more information about the issues on Windows, and the link I cited two days ago:

With comeonin, I've had several issues with developers finding it difficult just to set up the C environment on Windows. I usually advise them to install Visual Studio, after which the developer then has to follow this rather complex process. One issue particularly relevant to this PR is that, apparently, you need to run the command vcvarsall.bat amd64 every time the C code is compiled.

Some developers have reported that they also need to set the PATH manually, and other developers have had issues where the build fails because they are not using the correct prompt (even within Visual Studio).

We're trying to see if using gcc on Windows is a bit easier to work with. I presume that this will mean that we need to run make instead of nmake (after checking that gcc is present), but I'm still waiting for feedback on that.

@josevalim
Member

Thank you so much @riverrun. I believe if you want to use make, you would depend on something like MinGW? We have setup instructions for it here: https://github.com/elixir-lang/elixir/wiki/Windows

@jpmec
jpmec commented Mar 12, 2016

Hello, I would like to help out with the nmake portion of this pull request. Also I would really like to see it become available so that projects start using it. What's next to get this done?

@jpmec
jpmec commented Mar 12, 2016

Hello again, I have made some changes to compile.make.ex that can be seen here:

https://github.com/jpmec/elixir/tree/make-compiler-nmake

I can make a pull request for this if you wish, or you can just cut and paste (there really isn't that much there).

From my previous research into using nmake with comeonin, the key to nmake was setting environment variables before calling nmake. Some have suggested opening the VS command prompt, but IMHO this option sucks. For example, I like Powershell and I don't want to have to open a special command prompt just to compile my elixir and be forced to use that command prompt (especially when the only magic behind it is setting some env variables when it is opened).

I've made a project here that can get the environment variables needed for nmake

https://github.com/jpmec/exvc

So if we expose the make_env in the configuration, this will make it very easy to support nmake, or any other "special" compiler that needs some environment variables set before it can operate properly.

Anywho, hope you will consider it

@whatyouhide
Member

Hey @jpmec, I'm not sure about your solution. As far as I understand, you're trying to have some kind of "with this env" functionality by getting the env, modifying it and running make, then resetting it to its previous state. Your solution doesn't work properly though:

System.get_env["FOO"] #=> nil
env = System.get_env
System.put_env(%{"FOO" => "bar"})
System.get_env["FOO"] #=> "bar"
System.put_env(env)
System.get_env["FOO"] #=> "bar"

As you can see, you can only reset the values of existing variables; any new variable you introduce in make_env will not be reset by calling System.put_env/1 with the old env. We should rather save the value of just the variables introduced in make_env and reset only those (to nil if they didn't exist).

That said yes, we can support setting a make environment. I'm afraid too many make options are starting to pop out, I fear this is a smell that we're doing something wrong. Wdyt @josevalim, @lexmag and @ericmj?

@josevalim
Member
@jpmec
jpmec commented Mar 12, 2016

@whatyouhide you've interpreted it correctly, and yes I saw that it would pollute the env with new environment variables :-(. The System.cmd approach is probably better to restore the env completely.

I kinda agree about the "smell" for the compilers, but I don't know of an easy way to fix this. My desire would be to be able to plug in a compiler at my project mix level, so that the end user can wire up whatever compiler they need. However there is alot of change needed for that from what I could see. The approach you have done with the :make compiler fits the current pattern. I think the small change for the env would at least allow nmake to work, which would be a big win for Windows users new to elixir. I know that some like to use mingw instead of visual studio, so making sure that this can be configured at the top level is also definitely a need.

How would you like to proceed?

@jpmec
jpmec commented Mar 12, 2016

I've updated here

jpmec@05c7355

I simplified by using @josevalim suggestion about passing in the opts of System.cmd/3.
This was much nicer because no need to restore env afterward AFAIK.

@riverrun
Contributor
riverrun commented Apr 1, 2016

After testing a few different options with Visual Studio, I've updated the requirements page and the compiler for Comeonin. They now include a more straightforward way of getting set up on Windows.

Regarding this PR, I think the error messages in the Comeonin compiler provide a lot of useful information when things go wrong, and I think we should make this information more accessible - either by having generic error messages in the Compile.Make file or by adding the information to the documentation somewhere.

@josevalim
Member

@riverrun @whatyouhide @jpmec I have been thinking about this. This PR is here for 4 months already and there are still at least 2-3 months before Elixir v1.3. Furthermore, we may need to push improvements quickly after Elixir v1.3 is out which may not suit Elixir's release schedule.

What if we release this as a separate package, so we can iterate and continue improving this quickly, and by the time we are close to release Elixir v1.3, the package will be more stable and providing the sort of feedback that we require?

If we all agree, I will start a new package called elixir_make with a compiler called elixir_make which we will use for now. Once we bring into Elixir and Elixir v1.3 is out, all you need to do is ditch the elixir_make dependency and use the make compiler directly (as long as you are Elixir v1.3 only). The advantage of providing a package is also that if you want to support earlier Elixir versions, it will also be able to do so.

Thoughts?

@whatyouhide
Member

@josevalim this is what I did initially in https://github.com/whatyouhide/make_compiler, so I tend to agree. I stopped working on that when I opened this PR.

José, if you create the package I can re open a PR similar to this there.

@lexmag
Member
lexmag commented May 14, 2016

What if we release this as a separate package

I think that's great idea!

@josevalim
Member

Did you ever release any version for it, Andrea?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@whatyouhide
Member

@josevalim nope, #4082 was opened right after I pushed my library on GitHub so I never got around to publishing it to Hex (and I stopped working on it, so it's behind this PR feature-wise).

@josevalim
Member

So we can either create a new one or revamp that one, but since there was
not much work on there, I guess it does not matter much? :)

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@whatyouhide
Member
whatyouhide commented May 14, 2016 edited

@josevalim I have no preference, mine can definitely be ditched (I can kill the repo off completely) as there's very little work there, I have absolutely no problem with that. Maybe elixir-lang/* will give it more credibility 😃, but it's your call.

@josevalim
Member

Yeah, I was mostly thinking about elixir-lang too. :)

On Saturday, May 14, 2016, Andrea Leopardi notifications@github.com wrote:

@josevalim https://github.com/josevalim I have no preference, mine can
definitely be ditched (I can kill the repo off completely) as there's very
little work there, I have absolutely no problem with that. Maybe
elixir-lang/* will give it more credibility 😃, but it's your call.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4134 (comment)

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@whatyouhide
Member

@josevalim #makeithappen then! 👍

@josevalim
Member

I will create the project soon and give you access so you can push your
code and then I will work on the rest :)

On Saturday, May 14, 2016, Andrea Leopardi notifications@github.com wrote:

@josevalim https://github.com/josevalim #makeithappen then! 👍


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4134 (comment)

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@whatyouhide whatyouhide deleted the whatyouhide:make-compiler branch May 14, 2016
@whatyouhide
Member
whatyouhide commented May 14, 2016 edited

@josevalim sounds good, I can try working on it if you want (I have time this weekend) and if you have a clear-ish API in mind (I don't 😛).

@whatyouhide whatyouhide restored the whatyouhide:make-compiler branch May 14, 2016
@whatyouhide whatyouhide deleted the whatyouhide:make-compiler branch May 14, 2016
@josevalim
Member

@riverrun @jpmec We have posted updates on #4082. Please let us know how it goes. :)

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