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

Link against pthread #139

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Link against pthread #139

merged 1 commit into from
Mar 21, 2024

Conversation

qmuntal
Copy link
Collaborator

@qmuntal qmuntal commented Mar 21, 2024

We depend on pthread but are not explicitly adding it to the cgo LDFLAGS.

It used to work well because most linkers add stub implementations for old pthread functions when pthread is not linked. Unfortunately, pthread_setspecific was (correctly) added in #122, and that function is not normally stubbed, producing a link error under some unusual Go build configurations (Go links to pthread by default when using cgo).

See for example https://dev.azure.com/dnceng-public/public/_build/results?buildId=610701&view=logs&j=83130289-bc24-5ceb-b14e-d37ab0e8c945&t=77a3104e-0b46-5a5f-bd95-f1acb3049de6&l=634.

This PR adds the missing pthread entry in the #cgo LDFLAGS directive.

Copy link
Collaborator

@ueno ueno left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -2,7 +2,7 @@

package openssl

// #cgo LDFLAGS: -ldl
// #cgo LDFLAGS: -ldl -pthread
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a pedantic concern on hard-coding -pthread as some platforms seem to require -lpthread instead, though I guess it's out of scope of this module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm aware of this unfortunate conflict, but it is also truth that the Go toolchain uses -pthread when building cgo and nobody complained AFAIK. Let's keep it like this, we can always fix it if a bug is reported.

@qmuntal qmuntal merged commit 6b18634 into v2 Mar 21, 2024
16 checks passed
@qmuntal qmuntal deleted the remove-threadtests branch March 21, 2024 11:50
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.

3 participants