ErrorHandler trait not implemented errors after new major version releases of libcnb

[edmorley]

A new major version of libcnb was just released (0.3.0) since there had been breaking changes since the previous release.

Dependabot opened a PR against the JVM invoker buildpack
heroku/buildpacks-jvm#162

However CI failed with errors like:

error[E0277]: the trait bound `HerokuBuildpackErrorHandler<JvmFunctionInvokerBuildpackError>: ErrorHandler<JvmFunctionInvokerBuildpackError>` is not satisfied
  --> src/main.rs:25:9
   |
25 |         HerokuBuildpackErrorHandler::new(Box::new(handle_buildpack_error)),
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `ErrorHandler<JvmFunctionInvokerBuildpackError>` is not implemented for `HerokuBuildpackErrorHandler<JvmFunctionInvokerBuildpackError>`
   | 
  ::: /home/circleci/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/libcnb-0.3.0/src/runtime.rs:49:25
   |
49 |     error_handler: impl ErrorHandler<E>,
   |                         --------------- required by this bound in `cnb_runtime`

For more information about this error, try `rustc --explain E0277`.

(https://app.circleci.com/pipelines/github/heroku/buildpacks-jvm/599/workflows/4903b709-212d-4f3f-aa6b-a32a5d9ded63/jobs/6528)

This was puzzling, since the new version doesn't contain any changes to ErrorHandler:
Malax/libcnb.rs@v0.2.0...v0.3.0

Inspecting the PR diff of Cargo.lock, I noticed there were two versions of libcnb in use - 0.3.0 as a direct dependency of the JVM invoker buildpack, and then libcnb 0.2.0 as a dependency of libherokubuildpack.

The reason this fails is since there are now two versions of the ErrorHandler trait (even though they are technically identical), so the implemented version doesn't match the expected version.

There is an upstream tracking issue for rustc to output better diagnostics in the case of differing crate versions (rust-lang/rust/issues/22750). Several fixes have landed already, including rust-lang/rust/pull/66561 which was supposed to help with the "trait not implemented" variant - however it hasn't helped here.

The immediate solution for the compile errors above is to update libherokubuildpack's own dependency on libcnb to 0.3.0, and then update the JVM invoker buildpack's to the new versions of both at the same time - so there is only one version of libcnb in use.

However long term this approach is not ideal, since:

1. The rustc error messages are confusing, making this hard to debug whenever anyone encounters it.
2. It means every new major version release of libcnb then requires a dependency change + a new major release of libherokubuildpack, even though on the most part libherokubuildpack depends on very little of libcnb so won't actually have been incompatible.
3. After a new major release of libcnb/libherokubuildpack, every buildpack that depends on libcnb + libherokubuildpack will then have to update both simultaneously, which isn't supported by Dependabot, meaning time+effort of opening PRs manually (plus needing additional review, compared to being able to self-review Dependabot PRs).
4. New major versions of libcnb is something that will happen fairly regularly, since (a) it's needed each time the supported buildpack API version is bumped, (b) we need to have the flexibility to change the libcnb APIs as often as needed, particularly while we're still iterating on it.

Possible options (there may be more; I couldn't find much written in the community about this):

1. Document this issue in libherokubuildpack (but otherwise do nothing)
2. Change libherokubuildpack's dependency on libcnb to be less strict (eg >=0.3.0, since crates.io doesn't allow wildcard deps) - so that it will work with any version. However this would be a lie, plus also not clear whether cargo would favour trying to find a unified version across the dependency tree vs pulling in the latest, if a buildpack was pinned to an older libcnb.
3. Move the little functionality in libherokubuildpack that depends on libcnb into libcnb itself. Currently that functionality is not very Heroku specific, though I suppose that might change in the future (eg when we add metrics).
4. Expose libcnb via libherokubuildpack, and have buildpacks depend only on libherokubuildpack (ie: they wouldn't have a direct dependency on libcnb). This would mean libherokubuildpack is in control of the libcnb version.
5. Split libcnb's Traits/error handling out into another crate (kept in the libcnb repo using Cargo workspaces), which will only need a major version bump when there is a breaking Trait change (which will be very rarely). libherokubuildpack would depend only on that new crate and not libcnb itself. Though what happens if/when libherokubuildpack starts depending on even more of libcnb as we add more Heroku-specific functionality?

To me it seems (4) or (5) might be best?

@Malax @hone Thoughts?

[edmorley]

There is an upstream tracking issue for rustc to output better diagnostics in the case of differing crate versions (rust-lang/rust/issues/22750). Several fixes have landed already, including rust-lang/rust/pull/66561 which was supposed to help with the "trait not implemented" variant - however it hasn't helped here.

I've filed:
rust-lang/rust#89143

[Malax]

I'll close this since the ErrorHandler trait has been removed. Error handling works differently now and the issue should be gone.

[edmorley]

Trying now with libcnb 0.4.0, by using heroku/buildpacks-jvm/pull/215 and then updating its Cargo.toml to use a local libcnb only for the direct dependency (and not libherokubuildpack's dependency on libcnb):

libcnb = { path = "../../../libcnb.rs/libcnb", version = "0.4.0" }
libherokubuildpack = { version = "0.4.0", features = ["toml"] }

...I get:

$ cargo check
    Checking jvm-function-invoker-buildpack v0.0.0 (/Users/emorley/src/buildpacks-jvm/buildpacks/jvm-function-invoker)
error[E0308]: mismatched types
  --> src/main.rs:97:53
   |
97 |         handle_error_heroku(handle_buildpack_error, error)
   |                                                     ^^^^^ expected enum `libcnb::error::Error`, found enum `libcnb::Error`
   |
   = note: expected enum `libcnb::error::Error<JvmFunctionInvokerBuildpackError>`
              found enum `libcnb::Error<JvmFunctionInvokerBuildpackError>`
   = note: perhaps two different versions of crate `libcnb` are being used?

For more information about this error, try `rustc --explain E0308`.
error: could not compile `jvm-function-invoker-buildpack` due to previous error

So whilst version mismatches will still be an issue, at least the error message is better for types than it is for traits.

Longer term I think we should still consider option (4).