Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use features for minimal NIF version #537

Merged
merged 3 commits into from
May 25, 2023

Conversation

filmor
Copy link
Member

@filmor filmor commented May 14, 2023

  • Make rustler_sys forward compatible
  • Use Cargo features to define the minimal required NIF version
  • Stop using RUSTLER_NIF_VERSION in rustler_mix

Another side-effect of this change is that I have completely removed the Erlang call from the build process now as we do not attempt to auto-discover the highest available NIF version anymore. Instead, we compile by default for NIF 2.14, if anything higher is required, it has to be given explicitly as either RUSTLER_NIF_VERSION or by defining the respective feature.

I would suggest that we eventually (with rustler-0.29, rustler-sys-2.3.0) retire RUSTLER_NIF_VERSION in favour of users defining their rustler (or rustler-sys) dependency with the minimal required NIF version. This has also the huge advantage of being able to handle situations where rustler is used in a dependency gracefully.

@filmor filmor force-pushed the nif-version-features branch from cf48992 to 32eddce Compare May 14, 2023 17:59
@filmor filmor requested review from philss, scrogson, evnu and hansihe May 14, 2023 18:10
@filmor filmor marked this pull request as ready for review May 16, 2023 14:47
Copy link
Member

@philss philss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes totally sense to me! 👍

I think this way is much easier to control features based on the NIF version. My question is if we really need to deprecate the env var support.

My concern is that it's much easier today to set the NIF version by using the env var in the context of a precompilation workflow (we do it here).

The way using only features may lead to the need of defining custom features - in the user projects - to activate the correct nif_version_*_* on rustler, if the user is planning to support multiple NIF versions, right?

@filmor
Copy link
Member Author

filmor commented May 16, 2023

I'm not sure I get what you are concerned about. If a library itself supports multiple NIF versions (making use of newer functions if available), it should also put this support behind feature flags. The environment variable makes this handling much more error prone and leads to binaries that are needlessly xompiled to higher NIF version levels.

It's actually a simplification of the precompilation as in almost all cases it will suffice to provide a single binary to support all relevant BEAM versions.

@philss
Copy link
Member

philss commented May 17, 2023

It's actually a simplification of the precompilation as in almost all cases it will suffice to provide a single binary to support all relevant BEAM versions.

Got it. Makes sense!

@filmor filmor force-pushed the nif-version-features branch from 662d4f8 to a2ac3e3 Compare May 25, 2023 06:40
@filmor filmor force-pushed the nif-version-features branch from a2ac3e3 to 19ca337 Compare May 25, 2023 07:00
@filmor filmor merged commit 4ea76ec into rusterlium:master May 25, 2023
@filmor filmor deleted the nif-version-features branch May 25, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants