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

fix(profiling): allow to build on Alpine Linux #1391

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

jd
Copy link
Contributor

@jd jd commented Apr 29, 2020

This hides the pthread_t type usage and force the cast to unsigned long thread
no matter what.
This is fine with glibc and musl libc at least.

Fixes #1388

@jd jd requested a review from a team as a code owner April 29, 2020 13:09
@jd jd force-pushed the profiling/alpine-support branch from b3cb8f8 to 1a3a3db Compare April 29, 2020 13:09
@jd jd changed the title Pull request for profiling/alpine-support fix(profiling): allow to build on Alpine Linux Apr 29, 2020
brettlangdon
brettlangdon previously approved these changes Apr 29, 2020
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

should we add a note to our documentation "How to build from source" that at least lists the required system packages?

Maybe just something in readme is fine?

@@ -938,6 +949,7 @@ workflows:
- requestsgevent
- sqlalchemy
- sqlite3
- test_build_alpine
Copy link
Member

Choose a reason for hiding this comment

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

👍 thank you

Copy link
Member

Choose a reason for hiding this comment

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

I'd consider adding all versions we support on alpine, but this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, the compatibility issue is with their libc, not with any Python version specifically, so I don't think it's worth it.

This hides the pthread_t type usage and force the cast to unsigned long thread
no matter what.
This is fine with glibc and musl libc at least.

Fixes DataDog#1388
@jd
Copy link
Contributor Author

jd commented Apr 29, 2020

should we add a note to our documentation "How to build from source" that at least lists the required system packages?

Do you mean for Alpine, or for every Linux distro?

@brettlangdon
Copy link
Member

Do you mean for Alpine, or for every Linux distro?

Yes? It should mostly just be "you need python headers and a c++ compiler", example for apt-get install and example of apt add

probably don't need to be exhaustive... ?

or maybe not worth it?

@jd
Copy link
Contributor Author

jd commented Apr 29, 2020

I don't think it has been a problem so far. But if we ever get support/issues opened on this, it might be worth it.

(don't misread me, I don't want to sound like the guy who doesn't want to write documentation. 😆)

Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

lgtm! 😄

# ctypedef unsigned long pthread_t
# but e.g. musl libc defines pthread_t as a struct __pthread * which breaks the arithmetic Cython
# wants to do.
# We pay this with a warning at compilation time, but it works anyhow.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jd jd merged commit cfc1388 into DataDog:master Apr 30, 2020
@jd jd deleted the profiling/alpine-support branch April 30, 2020 10:12
@Kyle-Verhoog Kyle-Verhoog added this to the 0.37.1 milestone May 12, 2020
Kyle-Verhoog pushed a commit that referenced this pull request May 12, 2020
This hides the pthread_t type usage and force the cast to unsigned long thread
no matter what.
This is fine with glibc and musl libc at least.

Fixes #1388
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.

0.37.0 fails to build on Alpine Linux 3.9
3 participants