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

Bazel build of purego failing #144

Closed
eliottness opened this issue Jul 6, 2023 · 11 comments · Fixed by #147
Closed

Bazel build of purego failing #144

eliottness opened this issue Jul 6, 2023 · 11 comments · Fixed by #147

Comments

@eliottness
Copy link
Contributor

eliottness commented Jul 6, 2023

Bazel does not use the go usual build process for an increase in building speed. For this, they use their usual sandbox which makes the current working directory a random temporary directory and not the current package. This makes cross-package #include unable to work since the included file is not at the right place at build time. Here is the error:

external/com_github_ebitengine_purego/internal/fakecgo/asm_arm64.s:7: 
#include: open /var/folders/fn/j3_08pb94p9_hyz4myff84br0000gq/T/abi/abi_arm64.h: no such file or directory
compilepkg: error running subcommand external/go_sdk/pkg/tool/darwin_arm64/asm: exit status 1

The files where it fails are the following:

Purego is especially useful in Bazel environments because CGO is disabled by default. I checked on bazel side and this behaviour seems unavoidable. Do we know if cross-package usage of #include is really something that is officially supported (in which case bazel is at fault) and not a happy coincidence ? It is only a matter of copying the files in purego/internal/abi to purego/internal/fakecgo and purego/, in which case I can do the PR, or am I missing something ?

@hajimehoshi
Copy link
Member

Which Go version are you using?

@eliottness
Copy link
Contributor Author

eliottness commented Jul 6, 2023

Which Go version are you using?

@hajimehoshi 1.20.5. Is this related ?

@hajimehoshi
Copy link
Member

There is a known issue for Go 1.21 #140 so the version is an important information.

@TotallyGamerJet
Copy link
Collaborator

The assembler is poorly documented so if it's "officially" supported is mostly implementation defined. It's probably best to create an issue with Bazel to see if they'll fix it. Otherwise, I'm not sure how I feel about the copying since that would duplicate the abi headers.

@eliottness
Copy link
Contributor Author

I finally had the time to push this, we will see how they respond 👍

@eliottness
Copy link
Contributor Author

eliottness commented Aug 1, 2023

I looked into the project since the issues does not seems to be taken care of. And it seems that I will be really hard to make this work because each package is compiled in a separate sandbox. So this cross package include goes against the whole bazel sandboxing system.

@TotallyGamerJet I understand your concerns about copying the files and if I had any other working option I would present it to you but purego is a tool we intended to use on bazel in the first place, so it's a must on our side. Is there room for discussion ?

@eliottness
Copy link
Contributor Author

If the issue is about duplicating some code that was already copied from elsewhere, then we could build a small markdown table somewhere to reference all the files copied and where they come from. Just like you did in the README but more elaborate

@TotallyGamerJet
Copy link
Collaborator

TotallyGamerJet commented Aug 1, 2023

My concern was that if upstream Go changes the files then we have to remember to update it in all locations. However, I don't know any other solution and it's important to have bazel working

@eliottness
Copy link
Contributor Author

eliottness commented Aug 1, 2023

We could create a job scheduled every X than would git clone and diff the files. Just like we could find the dev build stream and try to run purego regularly on it.

@TotallyGamerJet
Copy link
Collaborator

That seems a little over-engineered for 4 files that don't change every release. I say we just copy them for now. You can make that PR if you want or wait until later for me to do it.

@eliottness
Copy link
Contributor Author

That seems a little over-engineered for 4 files that don't change every release. I say we just copy them for now. You can make that PR if you want or wait until later for me to do it.

I guess you are right. I have done the PR 👍

hajimehoshi pushed a commit that referenced this issue Aug 3, 2023
This change copies `abi_*.h` files and adjusts `#include` compilier
directives for Bazel builds.

The files `abi_*.h` and `internal/fakecgo/abi_*.h` are the same because
Bazel does not support cross-package use of `#include` so we need
each one once per package. See also
bazel-contrib/rules_go#3636.

Closes #144

Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
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 a pull request may close this issue.

3 participants