escript-related mix tasks #2974

Merged
merged 15 commits into from Feb 25, 2016

Projects

None yet

4 participants

@alco
Member
alco commented Dec 30, 2014

This PR implements a few additional escript-related mix tasks, outlined in this proposal.

Technical details

  • mix escript.install has been added that works similarly to mix archive.install: it takes a path or a URL to an arbitrary file and puts it under ~/.mix/escripts.
  • mix escript lists all executable files under ~/.mix/escripts.
  • mix escript.uninstall removes the file with the given name from ~/.mix/escripts.

Additional concerns

  • the ability to escript.install a prebuilt escript has been added because archive.install could already do that for .ez files, and it makes even more sense to have this work with escripts as they are not dependent on the installed Elixir version.
  • there is no environment variable MIX_ESCRIPTS because, as I mentioned, escript are generally independent of the installed Elixir version.

TODO

  • tests

  • to keep archive.install and escript.install symmetric, both should be able to fetch repositories and hex packages. The package will be fetched into a temporary directory, in the case of an escript it will also fetch dependencies. Then everything is compiled and subsequently archive.install or escript.install is called with the resulting archive or escript file. This makes it possible to host mix tasks on hex.pm too.

  • the command-line invocation syntax is just an idea. I have considered lots of alternatives, but would like to hear what the team thinks about it first. We can't really say that it is similar to the dep syntax in mix.exs because that would mean having something like this:

    mix escript.install phoenix
    mix escript.install phoenix 1.0.2
    mix escript.install phoenix github phoenixframework/phoenix
    

    I would much rather have the Mix task infer the source type from the URL given to it. The biggest problem is then to distinguish a local path from the package name.

  • if we allow both archives and escripts to be fetched from hex.pm, it may be confusing in which way a given package is supposed to be used: as a dependency, as an archive, or as an escript. Perhaps, hex.pm could indicate that visually in some way, or via the project's name.

@josevalim
Member

@alco Great work! I would like to suggest though to add the hex functionality in another pull request because I am considering backporting escript install to 1.0.x. I will add some specific comment to the PR overall soon.

@josevalim
Member

the ability to escript.install a prebuilt escript has been added because archive.install could already do that for .ez files, and it makes even more sense to have this work with escripts as they are not dependent on the installed Elixir version.

Perfect.

there is no environment variable MIX_ESCRIPTS because, as I mentioned, escript are generally independent of the installed Elixir version.

Makes sense! And MIX_HOME would already change it.

if we allow both archives and escripts to be fetched from hex.pm, it may be confusing in which way a given package is supposed to be used: as a dependency, as an archive, or as an escript. Perhaps, hex.pm could indicate that visually in some way, or via the project's name.

Honestly, it is very likely you don't want to install something as an archive. It will break with Elixir version, it is global and therefore has a chance of conflicting with your code, etc. Maybe we should simply not add this functionality to archives? I know they won't be symmetric then... but it will be for good reasons?

One final consideration: should we check if the ~/.mix/escripts is in your path after you install an escript and emit a warning if it is not? We should also update the windows installer to automatically add it to your $PATH and add a note to the homebrew one.

@josevalim josevalim and 1 other commented on an outdated diff Dec 31, 2014
lib/mix/lib/mix/tasks/escript.build.ex
@@ -108,7 +108,7 @@ defmodule Mix.Tasks.Escript.Build do
defp escriptize(project, language, force, should_consolidate) do
escript_opts = project[:escript] || []
- script_name = to_string(escript_opts[:name] || project[:app])
+ script_name = Mix.Escript.escript_name(project)
@josevalim
josevalim Dec 31, 2014 Member

Should we allow the -o option as in archive.build?

@alco
alco Jan 1, 2015 Member

I don't think it's useful for escripts. archive.build works with non-mix projects according to its docs. Escripts require mix.exs, so it would be simpler to have only one place where the name can be changed – in mix.exs.

@josevalim
josevalim Jan 2, 2015 Member

Good point, thanks!

@josevalim josevalim commented on the diff Dec 31, 2014
lib/mix/lib/mix/tasks/escript.install.ex
@@ -0,0 +1,113 @@
+defmodule Mix.Tasks.Escript.Install do
+ use Mix.Task
+
+ @shortdoc "Install an escript locally"
@josevalim
josevalim Dec 31, 2014 Member

Maybe we could have an Installer helper module that would abstract the functionality in the escript installer and archive installer? It seems there are a lot of similarities that we could refactor.

@alco
alco Jan 20, 2015 Member

I have extracted some pieces of functionality used by archive and archive.uninstall into Mix.Local.Utils. However, the installation processes for archives and escripts are quite different, I didn't try to unify them.

@alco
Member
alco commented Dec 31, 2014

Honestly, it is very likely you don't want to install something as an archive. It will break with Elixir version, it is global and therefore has a chance of conflicting with your code, etc. Maybe we should simply not add this functionality to archives? I know they won't be symmetric then... but it will be for good reasons?

Currently there are a few inconveniences related to how archives can be installed. First, the author has to build an archive and put it somewhere online for it to be installable with mix archive.install <url>. Second, if there is no .ez file hosted online, the user will need to clone the repository, build the archive (possibly changing the tag to match their Elixir version) and then run mix archive.install.

If mix archive.install supported fetching from hex.pm, then the author would only need to push to hex.pm, and the user would only need to mix archive.install <package name>. And if hex.pm kept track of Elixir version requirement for each package version, it could print warnings or automatically resolve to the supported package version.

In that sense, I see enabling support for packages in mix archive.install to have only benefits. Of course, archives remain brittle in the face of Elixir version updates. But if we allow to install them at all, installing from a hex package makes more sense than what we have now.

I like to have things like mix docs and mix dialyze installed system-wide so that they can be used with any project without prior configuration. They could be replaced with escripts, but since Mix tasks need to interact with the source code, they are already tied to a particular Elixir version. I know that your preference is to add Mix tasks as dependencies. We could go further and remove mix install.archive altogether. But if it remains, it should be able to fetch hex packages and, maybe, fetch repositories automatically.

@josevalim
Member

I like to have things like mix docs and mix dialyze installed system-wide so that they can be used with any project without prior configuration.

Right, that's the only reasonable use case. Even though, it can cause issues: what if we have ex_doc installed as an archive and a project list ex_doc as a dependency? Are we currently guaranteeing the package one has higher preference? Furthermore, depending on the order things run, I could end up using both versions like this:

  1. We run mix docs
  2. Since mix docs is in an archive, we start running it
  3. Because mix docs need to compile your code, it will fetch the dependencies and load the ex_doc version in the project
  4. mix docs fail because of different module versions :(

So there are a bunch of issues we should fix before really promoting archives. :(

@josevalim
Member

Also, thanks for the discussion: ❤️

@ericmj ericmj and 2 others commented on an outdated diff Jan 1, 2015
lib/mix/lib/mix/tasks/escript.install.ex
+
+ mix do escript.build, escript.install
+
+ The argument is usually a package name and optionally a version:
+
+ mix escript.install phoenix
+ mix escript.install phoenix 0.1.2
+
+ It could also be a remote repository:
+
+ mix escript.install git://example.com/foo.git # infers Git from the URL
+ mix escript.install https://example.com/foo.git # infers Git from the URL
+ mix escript.install github.com/example/foo # fetches git://github.com/example/example.git
+ mix escript.install https://github.com/example/foo # fetches https://github.com/example/example.git
+ mix escript.install --scm=git example.com/foo # fetches git://example.com/example
+ mix escript.install --scm=git https://example.com/foo # fetches https://example.com/example
@ericmj
ericmj Jan 1, 2015 Member

We should not infer git from a http url. It limits http urls to the git SCM but a lot of SCMs allow using http as transport.

@ericmj
ericmj Jan 1, 2015 Member

I still vote for the syntax proposed in the mailing list.

@alco
alco Jan 1, 2015 Member

In the case of https://example.com/foo.git above, Git would be inferred from the trailing .git part.

After some consideration I see the appeal of sticking with your proposal because of its similarity with dep syntax. It doesn't define a few things though. What is your take on these cases?

escript.install ./local_file        # ./ is used to distinguish from a hex package
escript.install path/to/local_file  # cannot be a hex package, so it can imply a local path
escript.install path path/to/local_repo  # ugly

# What about this? Should we forbid fetching remote scripts?
escript.install ??? https://example.com/escript_file

I realize that in the common case hex packages should be preferred, but we still have to define what happens with the other cases.

The ambiguity of supporting a prebuilt escript could be solved with a dedicated task:

escript.install-file local_file
escript.install-file path/to/local_file
escript.install-file http://example.com/escript

# or a flag

escript.install --file local_file
# etc...

It may not be all too important to be able to install prebuilt escripts, but leaving it out makes it feel incomplete.

What do you think, @josevalim, @ericmj ?

@ericmj
ericmj Jan 1, 2015 Member

If we are going to support prebuilt escripts I prefer these of your proposals:

escript.install ./local_file
escript.install path/to/local_file
escript.install https://example.com/escript_file

The following would build and install a local project (i.e. not a prebuilt file):

escript.install path path/to/local_repo
@josevalim
josevalim Jan 2, 2015 Member

I like @ericmj's proposal here. With no arguments it means to install prebuilt scripts, if you pass, hex|path|git, it means installing from a project (which therefore requires building and mix).

@josevalim
josevalim Jan 2, 2015 Member

I definitely think though we should move this into another discussion, as we can ship prebuilt installation earlier. :)

@alco
alco Jan 19, 2015 Member

I like @ericmj's proposal here. With no arguments it means to install prebuilt scripts, if you pass, hex|path|git, it means installing from a project (which therefore requires building and mix).

So have we settled on the following syntax?

# this PR
escript.install X    # X is a path or a URL to a prebuilt escript file

# future PR
escript.install X Y [Z]  
  # X is the package type (hex|path|git),
  # Y is the package name or URL
  # Z would be a version for a Hex package, or a ref for a Git repo

In that case, we can drop the ./ prefix when installing a file from the local directory. It was used to disambiguate from the mix escript.install <hex-package> syntax.

@josevalim
josevalim Jan 20, 2015 Member

I definitely like it. @alco, it is not clear if you like it though. Do you?

@alco
alco Jan 20, 2015 Member

I like it too. Will push an update tonight.

@ericmj ericmj commented on an outdated diff Jan 1, 2015
lib/mix/lib/mix/tasks/escript.install.ex
+ It could also be a remote repository:
+
+ mix escript.install git://example.com/foo.git # infers Git from the URL
+ mix escript.install https://example.com/foo.git # infers Git from the URL
+ mix escript.install github.com/example/foo # fetches git://github.com/example/example.git
+ mix escript.install https://github.com/example/foo # fetches https://github.com/example/example.git
+ mix escript.install --scm=git example.com/foo # fetches git://example.com/example
+ mix escript.install --scm=git https://example.com/foo # fetches https://example.com/example
+
+ Finally, it is also possible to install a local or remote escript that has already been built:
+
+ mix escript.install ./foo # local file (the ./ is necessary to disambiguate from a Hex package)
+ mix escript.install --file foo # alternative syntax to the above
+ mix escript.install path/to/foo # local file
+ mix escript.install https://example.com/foo # remote file
+ mix escript.install example.com/foo # remote file (tries https, then http)
@ericmj
ericmj Jan 1, 2015 Member

This is ambiguous, directories can also have dots in them.

@alco
Member
alco commented Jan 1, 2015

So there are a bunch of issues we should fix before really promoting archives. :(

@josevalim right, archives feel like they haven't been thought out very well. I'm not going to add any changes to them in this PR, but I will probably open another issue for them.

@alco
Member
alco commented Jan 20, 2015

Pushed an update that only implements installation of prebuilt escripts. Did some refactoring, added tests.

@alco alco changed the title from [WIP] escript-related mix tasks to escript-related mix tasks Jan 24, 2015
@josevalim
Member

@alco can you please rebase? Let's start work on this front. I have reviewed your changes and they look great, although I would try to unity the installer for both. It seems the only different is the path and that escripts need to call chmod? Thank you!

@alco
Member
alco commented Sep 14, 2015

I'll rebase later today, there are some conflicts I need to have a closer look at.

The differences in installing escripts and archives go beyond just having to call chmod. For archives we also check the Elixir version and forcibly remove previous ones. If you want to have a single main installation routine, it will need to have the ability to customize what to run before and after it.

@josevalim
Member

I was thinking this routing is the same https://github.com/elixir-lang/elixir/pull/2974/files#diff-2c79c79704bca75ad6388315cad90838R37 up to the should_install?. From that moment on, we can likely pass an anonymous function that would finish the job.

@alco
Member
alco commented Sep 15, 2015

Rebased. The biggest change I had to make during the rebase process was replacing Mix.Utils.copy_path! with Mix.Utils.read_path which I copied almost verbatim from archive.install. You can see it in the last commit. Feel free to make any further changes.

@josevalim
Member

@alco awesome! Can you please give a try at unifying both installation approaches? It will be specially important once we start building archives and escripts from git, so the sooner we unify, the best.

Also, what if we rename Mix.Local.Utils to Mix.Local.Installer?

@lexmag lexmag commented on an outdated diff Sep 15, 2015
lib/mix/lib/mix/local_utils.ex
@@ -0,0 +1,70 @@
+defmodule Mix.Local.Utils do
@lexmag
lexmag Sep 15, 2015 Member

Shouldn't be mix/local/utils.ex used as a name for a file?

@lexmag
lexmag Dec 30, 2015 Member

It is still a valid question. :bowtie:

@alco
Member
alco commented Sep 15, 2015

It's unlikely that I'll have time to work on this before the weekend, sorry. Feel free to take it or I'll get back to you next week.

@josevalim
Member

@alco oh, no worries at all. I forgot to say but this is not for 1.1, it will be for 1.2 or 1.3, which means we have at least 2 months ahead of us. :)

@alco
Member
alco commented Sep 15, 2015

OK. Are you planning to include install-from-git and/or install-from-hex in the same version?

@josevalim
Member

@alco it is your call. but given that we have some time, it would be nice to finally make progress on this (I am aware it stalled mostly due to me being unavailable though, sorry).

@alco
Member
alco commented Dec 30, 2015

Oh wow, this PR is more than a year old. Time flies X_X

Looking into it now.

@alco
Member
alco commented Dec 30, 2015

The latest Travis build has two issues that look unrelated to the changes in this PR:

15:17:51.138 [error] GenEvent handler GenEventTest.ReplyHandler installed in #PID<0.7394.0> terminating
** (RuntimeError) oops
    test/elixir/gen_event_test.exs:44: GenEventTest.ReplyHandler.handle_event/2
    (elixir) lib/gen_event.ex:1101: GenEvent.do_handler/3
    (elixir) lib/gen_event.ex:942: GenEvent.server_update/5
    (elixir) lib/gen_event.ex:927: GenEvent.server_notify/7
    (elixir) lib/gen_event.ex:900: GenEvent.server_event/4
    (elixir) lib/gen_event.ex:696: GenEvent.handle_msg/5
    (stdlib) proc_lib.erl:239: :proc_lib.init_p_do_apply/3
Last message: :raise
State: #PID<0.7393.0>

and

  1) test get and compile dependencies for rebar3 (Mix.RebarTest)
     test/mix/rebar_test.exs:179
     ** (RuntimeError) no shell process input given for yes?/1
     stacktrace:
       (mix) lib/mix/shell/process.ex:155: Mix.Shell.Process.yes?/1
       (mix) lib/mix/tasks/deps.compile.ex:146: Mix.Tasks.Deps.Compile.handle_rebar_not_found/1
       (mix) lib/mix/tasks/deps.compile.ex:130: Mix.Tasks.Deps.Compile.do_rebar3/2
       (mix) lib/mix/tasks/deps.compile.ex:66: anonymous fn/3 in Mix.Tasks.Deps.Compile.compile/1
       (elixir) lib/enum.ex:1088: Enum."-map/2-lists^map/1-0-"/2
       (mix) lib/mix/tasks/deps.compile.ex:53: Mix.Tasks.Deps.Compile.compile/1
       test/mix/rebar_test.exs:186: anonymous fn/0 in Mix.RebarTest.test get and compile dependencies for rebar3/1
       (elixir) lib/file.ex:1138: File.cd!/2
       test/mix/rebar_test.exs:182
@josevalim
Member

Master is passing. Is it a race condition or something wrong on rebase?

@alco
Member
alco commented Dec 30, 2015

The rebase was clean, without conflicts. I don't see those errors locally.

@lexmag lexmag commented on an outdated diff Dec 30, 2015
lib/mix/lib/mix/tasks/escript.ex
+
+ defp list_dir(path) do
+ case File.ls(path) do
+ {:ok, list} -> list
+ _ -> []
+ end
+ end
+
+ defp executable?(path) do
+ owner_exec_bit = 0o00100
+ group_exec_bit = 0o00010
+ other_exec_bit = 0o00001
+ stat = File.stat!(path)
+
+ (stat.mode &&& (owner_exec_bit ||| group_exec_bit ||| other_exec_bit)) != 0
+ and stat.type == :regular
@lexmag
lexmag Dec 30, 2015 Member

Style nitpicking: having and operator at the end of previous line might be better.
More context: elixir-lang/gettext@88c6f08#commitcomment-14914399.

@lexmag lexmag commented on an outdated diff Dec 30, 2015
lib/mix/lib/mix/tasks/escript.install.ex
+ install_escript(url_or_path, opts)
+ else
+ Mix.raise "Expected PATH to be a local file path or a file URL."
+ end
+
+ [] ->
+ project = Mix.Project.config
+ src = Mix.Escript.escript_name(project)
+ if File.exists?(src) do
+ install_escript(src, opts)
+ else
+ Mix.raise "Expected PATH to be given.\n#{usage}"
+ end
+
+ _ ->
+ Mix.raise "Unexpected arguments.\n#{usage}"
@lexmag
lexmag Dec 30, 2015 Member

Excess indentation.

@alco alco changed the title from escript-related mix tasks to [WIP] escript-related mix tasks Dec 30, 2015
@alco
Member
alco commented Dec 30, 2015

@josevalim I did some unification, still WIP. Will continue tomorrow.

@alco
Member
alco commented Dec 31, 2015

It's green now :shrug:. I wasn't able to reproduce the failure ** (RuntimeError) no shell process input given for yes?/1 mentioned above.

@alco
Member
alco commented Dec 31, 2015

@josevalim This is ready for review.

@josevalim josevalim added this to the v1.3.0 milestone Dec 31, 2015
@josevalim josevalim commented on an outdated diff Feb 21, 2016
lib/mix/lib/mix/archive.ex
@@ -9,21 +9,6 @@ defmodule Mix.Archive do
"""
@doc """
- Returns the archive name based on `app` and `version`.
@josevalim
josevalim Feb 21, 2016 Member

This is backwards incompatible. We need to keep it here.

@josevalim
Member

@alco thank you! I have added just one tiny comment, everything else is perfect (and we need to tidy up the FIXMEs in the code).

Also, we need to make sure this works on Windows because on Windows you need to use escript path/to/escript to run them: On Windows, we will probably need to use .bat files. See here: http://stackoverflow.com/questions/26569257/how-to-run-an-elixir-escript-on-windows-8-1

So I think we should likely generate .bat files alongside the escript files and just invoke them. Can you check how this would work on Windows? If for some reason you don't have a Windows machine around, let me know and we can merge this and I can tidy it up later on.

@alco alco changed the title from [WIP] escript-related mix tasks to escript-related mix tasks Feb 24, 2016
@alco
Member
alco commented Feb 24, 2016

@josevalim I have restored Mix.Archive.name/2 and removed that FIXMEs. Also rebased on master.

I don't have access to a Windows machine at the moment.

@josevalim josevalim commented on the diff Feb 25, 2016
lib/mix/lib/mix/local.ex
"""
- def archives_path do
- System.get_env("MIX_ARCHIVES") ||
- Path.join(Mix.Utils.mix_home, "archives")
+ @spec path_for(item) :: String.t
+ def path_for(:archive) do
+ System.get_env("MIX_ARCHIVES") || Path.join(Mix.Utils.mix_home, "archives")
+ end
+
+ def path_for(:escript) do
+ Path.join(Mix.Utils.mix_home, "escripts")
@josevalim
josevalim Feb 25, 2016 Member

Should we provide MIX_ESCRIPTS?

@lexmag
lexmag Feb 25, 2016 Member

Considering we have MIX_ARCHIVES 👍.

@alco
alco Feb 25, 2016 Member

This has already been asked and answered here (see the second quote).

@josevalim josevalim merged commit 25ca9c0 into elixir-lang:master Feb 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@josevalim josevalim commented on the diff Feb 25, 2016
lib/mix/lib/mix/tasks/escript.install.ex
+ ### Mix.Local.Installer callbacks
+
+ def check_path_or_url(_), do: :ok
+
+ def find_previous_versions(_src, dst) do
+ if File.exists?(dst), do: [dst], else: []
+ end
+
+ def before_install(_src, dst_path) do
+ File.rm(dst_path)
+ :ok
+ end
+
+ def after_install(dst, _previous) do
+ File.chmod!(dst, @escript_file_mode)
+ check_discoverability(dst)
@josevalim
josevalim Feb 25, 2016 Member

I would like to check this is actually a escript. Otherwise this command can be used to install any executable and it may be a bad idea.

@alco alco deleted the alco:alco/escript.install branch Feb 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment