Request: allow force_build=false for pre-releases

[mlwilkerson]

TL;DR: This is a request for a change to how the force_build option is set. It's not a bug report. I want to be able speed up build times that involve pre-releases by using pre-compiled NIFs.

---

According to the docs for 0.5.5, about the force_build option:

This is false by default, but if your :version is a pre-release (like "2.1.0-dev"), this option will always be set true

It goes on to show how it can otherwise be configured like this:

config :rustler_precompiled, :force_build, your_otp_app: true

I can confirm that rustler_precompiled works just as written there: no matter how I try to configure force_build as false, as long as I'm working with a pre-release version, force_build always ends up being true.

I can see the implementation here in config.ex:

force_build?: pre_release?(version) or Keyword.fetch!(opts, :force_build)

So there's no bug here.

But I would prefer to be able to set this option according to my needs, rather than having to release the package with a production version just to speed up my builds with pre-compiled NIFs.

It seems to be mixing concerns to insist on force_build: true for pre-releases, no matter what, regardless of all other configuration. It means I have to cut a production release of the package before it's ready, just to leverage the build time improvements that would otherwise be available through rustler_precompiled. Releasing a new production version might result in other packages picking up the package as "latest", even though I still consider it "under development" and not ready to be included in production application builds.

It may be significant to my use case that I'm trying to use rustler_precompiled for a private package. In my case, I own both the library code (the Elixir/rustler module), and the Elixir application code that depends upon that library. I want to speed up the build time of the application code when it depends on a pre-release version of the library.

It seems that rustler_precompiled is intended primarily for publicly released open source packages, as evidenced by the fact that the base_url is assumed to be publicly reachable when fetching pre-compiled NIFs (there's no way to configure authentication for the request in download_nif_artifact/1). I can imagine that for such public packages it might be a best practice, perhaps as a matter of security, to insist on force_build: true for any pre-released dependency.

Even if so, I wouldn't want such a best practice to make it impossible for me to use pre-compiled pre-release NIFs to speed up my private application builds that depend on my private NIF library.

It's normal for me, when iterating on a full stack feature that spans multiple code bases, to have one code base depend upon an Elixir/rustler package where both are under development. I need to be able to integrate all of the system components throughout a development cycle. I don't want to tag a NIF library with a production version until it's actually ready for production. That would probably result in its being taken up as the new "latest" version in the package management system and incorporated into production-ready application builds prematurely.

But I do want to get my builds running faster, because the development workflow involves building again, and again, and again, and again. It would boost productivity to speed up these build times, by allowing the application to build against a pre-built pre-release NIF library when I choose to do so.

[mlwilkerson]

If there's a solution to this that satisfies whatever other constraints there may be, I'm willing to implement it.

This is the sort of logic I'd have in mind. Changing this:

force_build?: pre_release?(version) or Keyword.fetch!(opts, :force_build)

to something like this:

force_build?: Keyword.get(opts, :force_build, pre_release?(version)) 

Meaning:

• if force_build is set at all in the opts, then use that setting.
• if force_build is not set in the opts, then the default value depends on whether it's a pre_release version
• by default, a pre_release version gets true, while a non-pre-release version gets false

However, I'm concerned that this would be a breaking change, because it means that for any current pre-release cases that do have non-default settings, the force_build behavior will change for them as a result of this change.

So another possibility might be: add an additional option whose presence changes this logic, but in whose absence the current logic remains. Maybe something like a bool called allow_precompiled_prerelease, and then the logic might be:

force_build?: if Keyword.get(opts, :allow_precompiled_prerelease, false), do: Keyword.get(opts, :force_build, pre_release?(version)), else: pre_release?(version) or Keyword.fetch!(opts, :force_build)

(Not sure whether I've got enough parentheses in there. Haven't tried to run it. Consider it pseudo-code.)

Thus, it would require opting in to this different logic.

[philss]

Hi @mlwilkerson 👋

I understand your concerns. I think we can add another option to control that behaviour.
Maybe the option can control if we force the build for pre releases.

force_build?: Keyword.fetch!(opts, :force_build) or Keyword.fetch!(opts, :force_build_for_pre_releases) and pre_release?(version)

And the default value of that option is true.

WDYT?

[mlwilkerson]

Thanks, @philss

I'll work up a PR.

I think the expression you wrote might need to be tweaked, though.

In testing that code, there's one case that fails, where:

• pre_release?(version) == true
• AND force_build: true
• AND force_build_for_pre_releases: false

I would expect that to produce false, because it's a pre-release and force_build_for_pre_releases: false, regardless of the value of force_build. But it actually produces true.

This handles all of the case as I'd expect:

    if pre_release?(version),
      do: Keyword.get(opts, :force_build_for_pre_releases, true),
      else: Keyword.fetch!(opts, :force_build)

And here's a test script showing the passing cases, using that logic:

defmodule Test do
  import ExUnit.Assertions

  def test do
    # honor current behavior
    refute(force_build?("1.0.0"))
    assert(force_build?("1.0.0", [force_build: true]))
    assert(force_build?("1.0.0-dev"))
    assert(force_build?("1.0.0-dev", [force_build: false]))
    assert(force_build?("1.0.0-dev", [force_build: true]))

    # allow new behavior with an additional option
    refute(force_build?("1.0.0-dev", [force_build: false, force_build_for_pre_releases: false]))
    refute(force_build?("1.0.0-dev", [force_build: true, force_build_for_pre_releases: false]))
    assert(force_build?("1.0.0-dev", [force_build: false, force_build_for_pre_releases: true]))
    assert(force_build?("1.0.0-dev", [force_build: true, force_build_for_pre_releases: true]))
    assert(force_build?("1.0.0-dev", [force_build_for_pre_releases: true]))
  end

  defp force_build?(version, opts \\ []) do
    opts = Keyword.merge([force_build: false], opts)

    if pre_release?(version),
      do: Keyword.get(opts, :force_build_for_pre_releases, true),
      else: Keyword.fetch!(opts, :force_build)
  end

  defp pre_release?(version) do
    "dev" in Version.parse!(version).pre
  end
end

Test.test()
IO.puts("ok")

[mlwilkerson]

Now, having worked through that, I realize something else:

The current implementation of pre_release?/1 only returns true when the pre-release value includes "dev". That's not what I would expect.

Admittedly, it is the example provided in the docs:

This is false by default, but if your :version is a pre-release (like "2.1.0-dev"), this option will always be set true.

However, a pre-release could also be "2.1.0-1" or "2.1.0-alpha", but neither of those would be considered pre-releases according to rustler_precompiled at this time.

In fact, in my specific case, my pre-release versions don't include "dev", which implies that rustler_precompiled should already be producing the behavior I wanted (I want force_build: false when the version is "0.12.0-1").

So while I still want to advocate for this change to separate these concerns, in my actual current problem I'm solving, I gotta go back to the drawing board!

And now I'm thinking there may be a separate but related issue: either the implementation of pre_release?/1 should be changed to produce true for any pre-release, or else the documentation should be updated to something like this:

This is false by default, but if your :version is a pre-release that includes "dev" (like "2.1.0-dev", not like "2.1.0-1"), this option will always be set true.

[philss]

@mlwilkerson sorry about the delay.

I think we should support more pre-releases, and also we should document that. WDYT?

[mlwilkerson]

I'd forgotten about this! I ended up solving my problem a different way and lost track of this concern. I'm tempted to close this issue, except that it might still be useful for others. And while I don't have time to submit a PR for it now, I'm still willing to do so later if it still seems worth implementing.

My solution (for now) has been to set the force_build option according to an environment variable, without regard to the version.

That works fine in my case. I have a growing number of internal/private packages that use rustler_precompiled. It's natural to have my build pipeline set environment variables. Perhaps that's not an elegant solution for public, open source packages, though. (I haven't though it through yet).