klarna / erlavro

Avro support for Erlang/Elixir (http://avro.apache.org/)
Apache License 2.0
131 stars 39 forks source link

snappyer introduces a c++ build requirement #115

Open mrjoelkemp opened 2 years ago

mrjoelkemp commented 2 years ago

Hey! Just wanted to flag that the use of snappyer in recent versions seems to now require a C++ toolchain in order to build erlavro.

This is fixable at the end-user level but breaking in docker environments. For example, with FROM hexpm/elixir:1.13.3-erlang-24.2.2-ubuntu-bionic-20210930 as builder, I'm now seeing a build error like:

10:33:49.099Z #13 40.78 ===> Compiling /app/deps/snappyer/c_src/snappy-sinksource.cc
10:33:49.099Z #13 40.78 ===> sh: exec: line 1: c++: not found
10:33:49.099Z #13 40.78 
10:33:49.099Z #13 40.80 ** (Mix) Could not compile dependency :snappyer, "/root/.mix/rebar3 bare compile --paths /app/_build/test/lib/*/ebin" command failed. Errors may have been logged above. You can recompile this dependency with "mix deps.compile snappyer", update it with "mix deps.update snappyer" or clean it with "mix deps.clean snappyer"
10:33:49.099Z #13 ERROR: executor failed running [/bin/sh -c mix do deps.get --only $MIX_ENV, deps.compile]: exit code: 1
10:33:49.099Z ------
10:33:49.099Z  > [ 9/15] RUN mix do deps.get --only test, deps.compile:
10:33:49.099Z ------
10:33:49.099Z executor failed running [/bin/sh -c mix do deps.get --only $MIX_ENV, deps.compile]: exit code: 1

I only flag this because I wasn't sure of the intentions and wanted to provide the datapoint. I went from 2.9.3 to 2.9.8 and saw this issue, which is unexpected for patch upgrades.

mikpe commented 2 years ago

I think that ship has sailed. Also lots of people, ourselves included, do have a C++ compiler in their builders, so won't notice this regression. I think the cleanest way out of this would be to make any compressor/decompressor a user-supplied module. That would put the burden of building and enabling such on the users and not the library itself. I'm not sure that's worth it just to eliminate a C++ requirement. Note most of our team is on vacation right now so responses here will be slow.

LostKobrakai commented 8 months ago

We've just updated erlavro in a nerves setup and also ran into issues with building snappyer, though more specifically with the cross compilation setup nerves provides. We're not using snappy compression, so making it an optional dependency does sound like a nice option to us. I also saw that snappy is an optional codec listed in the avro spec, so I don't think it would be unreasonable to handle it as optional from the erlavro perspective.