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 issue 189 #197

Merged
merged 7 commits into from
Jan 17, 2024
Merged

Fix issue 189 #197

merged 7 commits into from
Jan 17, 2024

Conversation

TotallyGamerJet
Copy link
Collaborator

Use the standard Cgo import mechanism when CGO_ENABLED=1. This fixes an issue where sometimes the external linker would fail to add the dlfcn symbols causing a linker error.

Closes #189

@TotallyGamerJet
Copy link
Collaborator Author

Once this gets merged I think it would be best to reorganize all files into internal/cgo and internal/nocgo so that the assembly and C files can have a clear distinction

Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

LGTM % comments

Should we cherry-pick this to 0.5?

cgo.go Outdated Show resolved Hide resolved
Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

Oh, can we have a test for this by the way?

Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

(Set the state to 'request changes' just in case)

@TotallyGamerJet
Copy link
Collaborator Author

I added a buildtest file. The GitHub workflow runs go build -v ./... which before this PR failed but with this PR works.

Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -0,0 +1,17 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Member

Choose a reason for hiding this comment

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

internal/buildtest would be better, as this doesn't have to be a public API to users.

Copy link
Collaborator Author

@TotallyGamerJet TotallyGamerJet Jan 17, 2024

Choose a reason for hiding this comment

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

none of the other C test folders are in internal. Should the test files be moved? not in this PR ofc.

Copy link
Member

@hajimehoshi hajimehoshi Jan 17, 2024

Choose a reason for hiding this comment

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

Now the other test files are in direct children packages. These are NOT Go packages, but these also should be in internal packages IMO. Let's move buildtest into internal, and consider moving the others later.

buildtest/main.go Outdated Show resolved Hide resolved
@hajimehoshi
Copy link
Member

Should we cherry-pick this to 0.5?

What do you think? (I think yes)

@TotallyGamerJet
Copy link
Collaborator Author

Should we cherry-pick this to 0.5?

What do you think? (I think yes)

As long as there are no merge conflicts I think so too.

Copy link
Member

@hajimehoshi hajimehoshi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@TotallyGamerJet TotallyGamerJet merged commit babd452 into ebitengine:main Jan 17, 2024
16 checks passed
@TotallyGamerJet TotallyGamerJet deleted the issue-189 branch January 17, 2024 15:43
hajimehoshi pushed a commit that referenced this pull request Jan 17, 2024
Use the standard Cgo import mechanism when CGO_ENABLED=1. This fixes an issue where sometimes the external linker would fail to add the dlfcn symbols causing a linker error.

Closes #189
hajimehoshi added a commit that referenced this pull request Jan 17, 2024
This reverts commit 579fc87.

Reason: go-vet fails:

```
Error: ./syscall_cgo_linux.go:14:33: undefined: cgo.Syscall9XABI0
Error: ./syscall_cgo_linux.go:25:13: undefined: cgo.Syscall9X
Error: Process completed with exit code 1.
```
hajimehoshi pushed a commit that referenced this pull request Jan 17, 2024
Use the standard Cgo import mechanism when CGO_ENABLED=1. This fixes an issue where sometimes the external linker would fail to add the dlfcn symbols causing a linker error.

Closes #189
hajimehoshi pushed a commit that referenced this pull request Jan 18, 2024
Use the standard Cgo import mechanism when CGO_ENABLED=1. This fixes an issue where sometimes the external linker would fail to add the dlfcn symbols causing a linker error.

Closes #189
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.

purgo build fail at linux
2 participants