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

Expose the package version in the C++ headers. #205

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

plietar
Copy link
Member

@plietar plietar commented Sep 6, 2024

On multiple occasions, I have made changes to individual that inadvertently broke the ABI of the native code. Unfortunately, after the new version of individual was released and users installed it, the R tooling wasn't able to detect this situation and did not automatically force malariasimulation to be re-installed or re-compiled. As a result, users were using a malariasimulation shared library that was built against individual 0.X together with an individual shared library at version 0.(X+1). This causes very subtle and hard to diagnose bugs, where one piece of code might read from the wrong place.

For example, #187 changed a return type of NumericVariable::get_values from a prvalue to a const-reference, avoiding unnecessary copies. For most intents and purposes, this is a API-compatible change, but the ABI of the method is different. After the update, if a user was still using a installation of malariasimulation that expected the old signature, the code would be compiled expecting to receive an std::vector by value (ie. three words representing the pointer to the data, the length and the capacity) instead received a pointer to such a vector.

Another example is #201, which removed the num_bits field and made it a compile-time constant. Code which used num_bits was now reading from an unintialized piece of memory instead. While the num_bits field was private, header-file code from the individual package that reads the field can make its way into the malariasimulation shared library and embed the assumptions about the layout of the class. Given the heavy use of templates in the package, almost all of it is re-instantiated and included in the downstream shared library.

In all these cases, recompiling malariasimulation is sufficient to ensure it uses the new ABI of the individual package. If the user uses devtools, then this simply requires them to call devtools::clean_dll. The issue is that identifying the problem is near impossible for a user. Nothing in the behaviour of malariasimulation points to an incompatibility, instead the package only misbehaves in very suprising ways.

The goal of this change is to export the version of the individual package in its C++ headers. The constant may then be included and compiled into the malariasimulation shared object. At load-time, malariasimulation can compare this version with the result of packageVersion("individual") and show a warning or an error if they don't match. The user will be directed to re-install malariasimulation.

I could not figure out a way of exporting the version from DESCRIPTION into header files. It's possibly something Rcpp::compileAttributes() could do, but it doesn't. Instead the version number is duplicated and a CI check ensures that they are kept in sync.

Some alternatives I considered instead:

  1. Don't make ABI breaking changes to individual: I think this is a non-starter. We do not want to restrict our abilities to improve and optimise the package. Moreover identifying what is or isn't a breaking change is quite challenging.
  2. Stop exposing a C++ API, and require users to call the R functions instead: allowing users to write high-performant processes or utility functions for the hot path of their models is quite an important aspect of individual.
  3. Break the ABI but announce it in release notes so users know to be on the look out and to re-compile their copy of malariasimulation: no one reads release notes, especially for a package they only use indirectly. Moreover it still requires identifying ABI breaking changes.
  4. Implement the suggested scheme, but use an ABI version instead of the package version. This will avoid false positives, where the package's version number is increased even though the ABI hasn't changed. I think the rate of releases and the cost of recompiling malariasimulation are low enough that it is better to err on the safe side and treat every release as if it could be ABI breaking.

On multiple occasions, I have made changes to individual that
inadvertently broke the ABI of the native code. Unfortunately, after the
new version of individual was released and users installed it, the R
tooling wasn't able to detect this situation and did not automatically
force malariasimulation to be re-installed or re-compiled. As a result,
users were using a malariasimulation shared library that was built
against individual 0.X together with an individual shared library at
version 0.(X+1). This causes very subtle and hard to diagnose bugs,
where one piece of code might read from the wrong place.

For example, #187 changed a return type of
`NumericVariable::get_values` from a prvalue to a const-reference,
avoiding unnecessary copies. For most intents and purposes, this is a
API-compatible change, but the ABI of the method is different. After the
update, if a user was still using a installation of malariasimulation
that expected the old signature, the code would be compiled expecting to
receive an `std::vector` by value (ie. three words representing the
pointer to the data, the length and the capacity) instead received a
pointer to such a vector.

Another example is #201, which removed the `num_bits`
field and made it a compile-time constant. Code which used `num_bits`
was now reading from an unintialized piece of memory instead. While the
`num_bits` field was private, header-file code from the individual
package that reads the field can make its way into the malariasimulation
shared library and embed the assumptions about the layout of the class.
Given the heavy use of templates in the package, almost all of it is
re-instantiated and included in the downstream shared library.

In all these cases, recompiling malariasimulation is sufficient to
ensure it uses the new ABI of the individual package. If the user uses
`devtools`, then this simply requires them to call
`devtools::clean_dll`. The issue is that identifying the problem is near
impossible for a user. Nothing in the behaviour of malariasimulation
points to an incompatibility, instead the package only misbehaves in
very suprising ways.

The goal of this change is to export the version of the individual
package in its C++ headers. The constant may then be included and
compiled into the malariasimulation shared object. At load-time,
malariasimulation can compare this version with the result of
`packageVersion("individual")` and show a warning or an error if they
don't match. The user will be directed to re-install malariasimulation.

I could not figure out a way of exporting the version from `DESCRIPTION`
into header files. It's possibly something `Rcpp::compileAttributes()`
could do, but it doesn't. Instead the version number is duplicated and a
CI check ensures that they are kept in sync.

Some alternatives I considered instead:
1. Don't make ABI breaking changes to individual: I think this is a
   non-starter. We do not want to restrict our abilities to improve and
   optimise the package. Moreover identifying what is or isn't a breaking
   change is quite challenging.
2. Stop exposing a C++ API, and require users to call the R functions
   instead: allowing users to write high-performant processes or utility
   functions for the hot path of their models is quite an important
   aspect of individual.
3. Break the ABI but announce it in release notes so users know to be on
   the look out and to re-compile their copy of malariasimulation: no one
   reads release notes, especially for a package they only use
   indirectly. Moreover it still requires identifying ABI breaking changes.
4. Implement the suggested scheme, but use an ABI version instead of the
   package version. This will avoid false positives, where the package's
   version number is increased even though the ABI hasn't changed. I
   think the rate of releases and the cost of recompiling malariasimulation
   are low enough that it is better to err on the safe side and treat
   every release as if it could be ABI breaking.
@plietar plietar changed the title Expose package version in the C++ headers. Expose the package version in the C++ headers. Sep 6, 2024
@plietar
Copy link
Member Author

plietar commented Sep 6, 2024

See mrc-ide/malariasimulation#332 for the malariasimulation side.

Steps to test:

remotes::install_github('mrc-ide/individual@expose-version', upgrade=FALSE) # Install this PR
remotes::install_github('mrc-ide/malariasimulation@check-individual-version', upgrade=FALSE) # Install the malariasimulation PR
remotes::install_github('mrc-ide/individual@expose-version-next', upgrade=FALSE) # Install a version of this PR that also bumps the version number

library(malariasimulation)
#> Warning message:
#> In fun(libname, pkgname) :
#>  malariasimulation was compiled against version '0.1.17' of the individual, but version '0.1.18' is currently loaded. This is likely to cause malariasimulation to misbehave. You should re-install malariasimulation. If using devtools, run `devtools::clean_dll` and then load malariasimulation again.

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.

1 participant