philss / rustler_precompiled

Use precompiled NIFs from trusted sources in your Elixir code
181 stars 25 forks source link

Inverted force_build priority for otp_app? #80

Open paulgoetze opened 1 month ago

paulgoetze commented 1 month ago

The current implementation prioritizes a force_build option from a NIF project "project_a" (as used in the example project) over the config :rustler_precompiled, :force_build, project_a: true config defined in "project_b" (which uses "project_a" as a dependency).

Hence, if I understand it correctly, configuring config :rustler_precompiled, :force_build, project_a: true in this case currently does not have any effect, because as soon as a force_build option is present, it can not be overwritten anymore (using Keyword.put_new here https://github.com/philss/rustler_precompiled/blob/main/lib/rustler_precompiled.ex#L161)

Is this behaviour on purpose?

philss commented 1 month ago

The intention was to prioritize the config that is set directly from the module definition. But thinking now, your proposal makes sense. We could make the application config a priority in case it is set, otherwise we leave as it is. WDYT? And would you mind to send a PR?

paulgoetze commented 1 month ago

:+1: Yes, I think that makes sense, I'll send a PR.

paulgoetze commented 1 month ago

Hm, now that I thought about it again, I must say that your current implementation actually appears to be more intuitive to me. Using the option if provided in the module and else falling back to the application config might actually be more predictable in how the force_build option works.

So I'll backpedal here and advocate for that it should be solved on the NIF Module level. :sweat_smile:

Just as a reference, for the rustler_precompiled example app that would be something like:

# lib/rustler_precompilation_example/native.ex

  opts = [
    otp_app: :rustler_precompilation_example,
    # ...other rustler_precompiled options
  ]

  use RustlerPrecompiled,
    (if System.get_env("RUSTLER_PRECOMPILATION_EXAMPLE_BUILD") in ["1", "true"] do
      Keyword.put(opts, :force_build, true)
     else
      opts
    end)

which only passes the force_build option if the ENV var is actually given (here as "1" or "true", but this logic is then up to the NIF creator).