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

Allow avro C to be built on musl based systems. #740

Merged
merged 1 commit into from
Dec 18, 2019
Merged

Allow avro C to be built on musl based systems. #740

merged 1 commit into from
Dec 18, 2019

Conversation

titouanc
Copy link

@titouanc titouanc commented Dec 8, 2019

Currently, the avro C library can not be built for Linux systems based on the musl standard C library, which is often used on embedded systems. See the full commit message for more details.

This PR has been tested against the following C libraries, on x86 and arm architectures:

  • uClibc
  • musl
  • glibc

Make sure you have checked all steps below.

Jira

  • My PR does not have an associated JIRA issue

Tests

  • My PR does not add new tests, as it fixes a compilation issue within a particular build environment that is currently not reproducible in Travis

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

The type `ssize_t` is defined in sys/types.h, and nowhere else
in the musl standard C library, so it should be included for the
compilation to succeed.

This fixes several errors like:

    In file included from src/generic.c:29:0:
    src/generic.c: In function ‘avro_generic_value_new’:
    src/avro_generic_internal.h:63:39:
        error: ‘ssize_t’ undeclared (first use in this function);
               did you mean ‘size_t’?

Signed-off-by: Titouan Christophe <titouan.christophe@railnova.eu>
@probot-autolabeler probot-autolabeler bot added the C label Dec 8, 2019
@kojiromike
Copy link
Contributor

It should be possible to test this with an Alpine container. I took a crack at it, but I wasn't able to successfully build avro. It's probably more to do with my own ignorance of the build system.

@titouanc
Copy link
Author

titouanc commented Dec 11, 2019

@kojiromike indeed, there are a few required packages to be able to compile within Alpine:

  • bash (required by shebang lines in helper scripts)
  • gcc (and g++ is also required somehow by cmake, not sure why)
  • make, cmake
  • jansson-dev and musl-dev (shared libs + headers needed)

I succeeded to build the C part with this Dockerfile (placed at the toplevel):

FROM alpine:latest

WORKDIR /root

RUN apk update
RUN apk add bash gcc g++ make cmake jansson-dev musl-dev

COPY . .

WORKDIR /root/lang/c
RUN mkdir build && cd build && cmake .. && make

Eventually running the same Dockerfile without the patch in this PR would not succeed

@RyanSkraba
Copy link
Contributor

RyanSkraba commented Dec 11, 2019

Hello! This is the one-liner I used for musl:

docker run --rm -it -v $PWD:$HOME/avro -w $HOME/avro alpine:3.7 sh -c "apk add g++ cmake bash jansson-dev make && cd lang/c && ./build.sh test"

I can confirm that it fails without the change in this PR, and succeeds with the change.

I know for glibc (like the build process uses), we can do the equivalent:

docker run --rm -it -v $PWD:$HOME/avro -w $HOME/avro gcc:9.2 bash -c "apt-get update -y && apt-get -y install cmake libjansson-dev && cd lang/c && ./build.sh test"

I'm not sure about uClibc.

@titouanc
Copy link
Author

I tested a uClibc-ng build within Buildroot (which is by the way where this patch originated 🙂) and it succeeds. However, I did not find a Docker image based on uClibc after a quick Google search.

@titouanc
Copy link
Author

Hello @kojiromike and @RyanSkraba , is there anything needed to make this move forward ? Please let me know if I can do anything to help

@kojiromike
Copy link
Contributor

I was able to run this on alpine and confirm what Ryan said. However, I'm concerned that we don't have any automated test set up for TravisCI, so any future committer could accidentally break musl support again and not realize it.

@RyanSkraba
Copy link
Contributor

RyanSkraba commented Dec 18, 2019

Oh, I forgot to mention: I checked uClibc with an 4 year old version of alpine (not super satisfactory or confident about it):

docker run --rm -it -v $PWD:$HOME/avro -w $HOME/avro alpine:2.7 sh -c "apk update && apk add g++ cmake bash jansson-dev make && cd lang/c && ./build.sh test"

What do you think about merging this and creating a JIRA about improving the build with a "secondary" check on lang/c&musl? It would be consistent with the lang/csharp&windows check.

Edit: I created https://issues.apache.org/jira/browse/AVRO-2660.

Copy link
Contributor

@kojiromike kojiromike left a comment

Choose a reason for hiding this comment

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

@RyanSkraba Sounds good to me. Do you want to do the honors?

@RyanSkraba
Copy link
Contributor

@RyanSkraba Sounds good to me. Do you want to do the honors?

I'd like to practice, if that's OK :D I'm on it!

@RyanSkraba RyanSkraba merged commit 9b39a98 into apache:master Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants