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

Alpine Linux compatibility for pony #1844

Merged
merged 1 commit into from
Jul 20, 2017
Merged

Conversation

dipinhora
Copy link
Contributor

Changes related to getting pony compiling and running on Alpine
Linux Edge.

  • Define _GNU_SOURCE so musl glibc compatibility is enabled.
  • Makefile changes to detect Alpine Linux and link in libexecinfo
  • Changes to stop using glibc specific __GNUC_PREREQ macro
  • Changes to genexe linking to link in libexecinfo if on Alpine
    or FreeBSD (this is likely also needed but untested)
  • Changes to move Linux cpu affinity setting outside of thread
    creation function and into same set affinity function as for
    other platforms. This also includes enabling asio thread affinity
    setting for non-linux platforms.
  • Rename SCHED_BATCH to PONY_SCHED_BATCH to avoid conflict with
    musl.
  • Set this_scheduler thread-local variable to NULL on
    ponyint_sched_init to avoid segfault in running codegen test
    suite.

Resolves #1729

NOTE: This is unlikely to make pony compatible with all musl based distributions/compiling attempts because pony still relies on execinfo.h and musl doesn't provide that. Some distributions include libexecinfo (as Alpine did) but others (ubuntu for example) may not. According this post (http://www.openwall.com/lists/musl/2015/04/10/2) in the musl mailing list, the appropriate longer term solution might be to use libunwind.

/cc @sylvanc (for a review of the affinity changes in particular and everything in general)
/cc @Praetonus (for a review of the this_scheduler and execinfo stuff in particular and everything in general)


Tested using the following in docker run --privileged -it --rm alpine:edge sh on an Ubuntu 16.04 VM:

@@ -277,6 +278,9 @@ static bool link_exe(compile_t* c, ast_t* program,
#ifdef PONY_USE_LTO
"-flto -fuse-linker-plugin "
#endif
#if defined(PLATFORM_IS_ALPINE_LINUX) || defined(PLATFORM_IS_FREEBSD)
Copy link
Member

Choose a reason for hiding this comment

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

Quick thought: could PLATFORM_IS_ALPINE_LINUX be named PLATFORM_IS_MUSL_LINUX, and still be correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jemc This is unlikely to be correct. See the note in my main PR comment about libexecinfo/execinfo.h. Additionally, there is no way to detect that the libc being used is musl (see: http://wiki.musl-libc.org/wiki/FAQ#Q:_why_is_there_no_MUSL_macro_.3F).

The way this is currently detected is via a Makefile check for the existence of /etc/alpine-release and is tied to the Alpine Linux distribution.

Copy link

@shizmob shizmob Apr 19, 2017

Choose a reason for hiding this comment

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

Highly disagree with this, as it also breaks cross-compilation. This should be properly detected through a test checking if a symbol in <execinfo.h> can be resolved by the linker without any additional libraries, and then with -lexecinfo if it can't.

Copy link
Contributor Author

@dipinhora dipinhora Apr 19, 2017

Choose a reason for hiding this comment

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

@shizmob I agree that the best solution is to do a test and then base a decision on that however that was going to add a level of complexity into the ponyc linking code that I wasn't comfortable with. If others agree that's the route desired, then we can implement the appropriate solution.

Can you give me an example set of steps/commands to reproduce where it breaks cross-compilation?

@killerswan
Copy link
Member

Cooool!

@sylvanc
Copy link
Contributor

sylvanc commented Apr 19, 2017

libexecinfo is for generating backtraces. We do indeed need it in the makefile for ponyc and the tests and benchmarks, for pony_assert. But I don't think we need it when building pony programs, so I think it can be removed, fixing the cross-compilation problem and eliminating the PLATFORM_IS_ALPINE_LINUX flag.

@dipinhora
Copy link
Contributor Author

@sylvanc Maybe I'm misunderstanding things but pony_assert is used in parts of libponyrt in addition to libponyc. This means that programs compiled using ponyc and linking against libponyrt have references to the backtrace and backtrace_symbols functions and fail with the following errors (these errors are the reason I added in the logic for libexecinfo and the PLATFORM_IS_ALPINE_LINUX define):

Linking ./stdlib
Warning: environment variable $CC undefined, using cc as the linker
src/libponyrt/platform/ponyassert.c:42: error: undefined reference to 'backtrace'
src/libponyrt/platform/ponyassert.c:43: error: undefined reference to 'backtrace_symbols'
collect2: error: ld returned 1 exit status

I agree that the PLATFORM_IS_ALPINE_LINUX define is not ideal. Maybe we need to change the use of pony_assert in libponyrt? Or maybe an alternate solution might be to use libunwind instead of execinfo.h as that musl mailing list post suggests?

@dipinhora dipinhora force-pushed the PR_musl branch 2 times, most recently from f1cd4d0 to 9d79bd6 Compare April 19, 2017 23:01
@dipinhora
Copy link
Contributor Author

@sylvanc I've made a change to how the #if condition for pony_assert works to ensure that pony_assert for libponyrt maps to normal assert instead of the custom pony_assert_fail that prints backtraces. This means that we are back to the old behavior for libponyrt where there are no assertions or backtraces for release builds and normal assert based assertions for debug builds. This should have no impact on the libponyc assertions with backtraces using the new pony_assert_fail logic.

This has removed the dependency on the backtrace functions in libponyrt so I have removed the define for PLATFORM_IS_ALPINE_LINUX and the associated logic to link in libexecinfo. This should resolve the concerns related to the define and cross compiling.

Please let me know if this is an acceptable solution or if you have further suggestions for improving things.

@kaniini
Copy link

kaniini commented Apr 24, 2017

hi, i merged the llvm changes needed in alpine. are you guys interested in integrating ponyc into alpine itself officially, yet?

@sylvanc
Copy link
Contributor

sylvanc commented May 3, 2017

@kaniini, we are super interested in integrating ponyc into alpine itself officially! Sorry for the late response on this. What's the process?

@sylvanc
Copy link
Contributor

sylvanc commented May 3, 2017

@dipinhora this looks great! Let's merge it.

@kaniini
Copy link

kaniini commented May 3, 2017

@sylvanc right now just somebody showing up with an APKBUILD willing to maintain the packaging. we prefer upstream devs (or those with close ties to upstreams) where possible.

@jemc
Copy link
Member

jemc commented May 10, 2017

@kaniini - thanks for the info. I don't know if anyone is in a good position to step up and volunteer for maintaining the APKBUILD, but it's something I might do at some point in the future, if no one else has stepped up before then.

@jemc
Copy link
Member

jemc commented Jul 17, 2017

Is there a reason this hasn't been merged yet?

@SeanTAllen
Copy link
Member

@dipinhora can you rebase this so we can merge?

Changes related to getting pony compiling and running on Alpine
Linux Edge.

* Define `_GNU_SOURCE` so musl glibc compatibility is enabled.
* Makefile changes to detect Alpine Linux and link in `libexecinfo`.
* Makefile changes to define PONY_NO_ASSERT for libponyrt-pic.
* Changes to stop using glibc specific `__GNUC_PREREQ` macro.
* Changes to move Linux cpu affinity setting outside of thread
  creation function and into same set affinity function as for
  other platforms. This also includes enabling asio thread affinity
  setting for non-linux platforms.
* Rename `SCHED_BATCH` to `PONY_SCHED_BATCH` to avoid conflict with
  musl.
* Set `this_scheduler` thread-local variable to `NULL` on
  `ponyint_sched_init` to avoid segfault in running codegen test
  suite.
* Change `#if` for `pony_assert` to use normal `assert`
  for builds that define `PONY_NO_ASSERT` (`libponyrt` and
  `libponyrt-pic` only as of this commit) instead of the custom
  `pony_assert_fail` that prints backtraces.

Resolves ponylang#1729
@dipinhora
Copy link
Contributor Author

Rebased.

Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@SeanTAllen SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for musl-based Linux (e.g. Alpine)
8 participants