elixir_make icon indicating copy to clipboard operation
elixir_make copied to clipboard

Improvements to precompiler

Open josevalim opened this issue 3 years ago • 4 comments

Hi @cocoa-xu!

I have been looking at the code and I think we can simplify the precompiler implementation if we move the logic for zipping/checksuming to elixir_make.

The idea would be to roughly have precompile return a something like {:ok, [{target, cache_dir}]}. Then we can go through those lists and tar.gz and add the relevant metadata. By doing this, I think we can simplify the precompile callback. I also think we can remove both available_nif_urls() and current_target_nif_url callbacks, as long as we add a make_precompiled_url to elixir_make.

The idea is to make the precompiler worry only about precompilation and none of the actual artefact management.

WDYT?

josevalim avatar Aug 17 '22 14:08 josevalim

Hi @josevalim, thank you very much for reviewing the code. Here are some of my thoughts!

we can simplify the precompiler implementation if we move the logic for zipping/checksuming to elixir_make.

Yes, I think that seems to be better. Perhaps I can first explain why I designed it in this way. So the original design considered that

  • perhaps the precompiler could use its preferred archiving algorithm/tools such as LZMA or 7zip because they have better compression ratios than tarballs.
  • for a (probably) small portion of users, this allows they to use encrypted artefacts or some other auth mechanisms (such as HTTP basic auth) when fetching and/or unarchiving the artefacts
    • using their private hex registry or their private pipeline
    • having a paid version of their libraries

I think maybe we can make download_or_reuse_nif_file/1 as an optional callback. And the precompiler can return a list from available_nif_urls with each element to be either

  • {target, nif_url}
  • {:custom, target, nif_url}

For the first case, we follow everything you described here and for the second case, we can pass the tuple to the precompiler's download_or_reuse_nif_file/1 and let the precompiler fetch and unarchive it to the priv directory.

The idea would be to roughly have precompile return a something like {:ok, [{target, cache_dir}]}. Then we can go through those lists and tar.gz and add the relevant metadata.

Yes, I agree on this one.

And if we are going to do the optional download_or_reuse_nif_file/1 thing, then the precompiler should return {:ok, [{target, archived_file_path}]}.

I also think we can remove both available_nif_urls() and current_target_nif_url callbacks, as long as we add a make_precompiled_url to elixir_make.

I totally agree on this one.

I wonder if the value of make_precompiled_url should be a template string or not? for some examples

"https://github.com/name/repo/downloads/release/<% =version %>/<% =artefact_filename %>"
"https://example.com/downloads/release/<% =artefact_filename %>?version=<% =version %>"

The idea is to make the precompiler worry only about precompilation and none of the actual artefact management.

By making the changes described above (returning {:custom, target, nif_urls} and the optional download_or_reuse_nif_file/1 callback), I think the precompiler can choose if it would like to touch the artefact management part, and by default, it can only worry about the precompilation part.

WDYT?

cocoa-xu avatar Aug 17 '22 19:08 cocoa-xu

perhaps the precompiler could use its preferred archiving algorithm/tools such as LZMA or 7zip because they have better compression ratios than tarballs. for a (probably) small portion of users, this allows they to use encrypted artefacts or some other auth mechanisms (such as HTTP basic auth) when fetching and/or unarchiving the artefacts

Then I would rather support or find a way to implement those features in elixir_make, so all precompilers can leverage them. Otherwise we are in a spot where each precompiler would have to implement those features individually. For example, we could have make_precompile_compressor in the future. Or make_precompile_downloader. And so on. :)

I wonder if the value of make_precompiled_url should be a template string or not? for example

Sounds good! I think we use %{path} and %{file} in ExDoc, so perhaps we could use the same here?

josevalim avatar Aug 17 '22 19:08 josevalim

Okay, I think I got it! So in summary, we need to

  • have precompile return something like{:ok, [{target, cache_dir}]} then in elixir_make we go through the list, compress the corresponding cache directory using :erl_tar and add the relevant metadata.
  • remove the available_nif_urls and current_target_nif_url callbacks
  • use a template make_precompiled_url
  • make download_or_reuse_nif_file as part of the elixir_make instead of as a callback. In the future, this download_or_reuse_nif_file thing can be a callback for the make_precompile_downloader.

Is this correct?

cocoa-xu avatar Aug 17 '22 20:08 cocoa-xu

Perfect!

josevalim avatar Aug 17 '22 20:08 josevalim