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

Enhancement: arm64 #42

Merged
merged 8 commits into from
May 18, 2023
Merged

Enhancement: arm64 #42

merged 8 commits into from
May 18, 2023

Conversation

tigh-latte
Copy link
Contributor

@tigh-latte tigh-latte commented May 16, 2023

A move to solve #39.

Add a detect_arm64.go file that doesn't import guesslang, and add build flags to only build this variant on arm64 architecture.

Also, add a Dockerfile for building an arm64 variant (note this can only be built on an arm64 machine, such as an rpi. It would be nice if we could build for both archs using the --platform arg. Happy to hear thoughts on this.)

I have this running on an rpi that's public facing at the moment that you can test on to see how this performs with the guesser disable, just give me a shout and I'll message details.

Let me know if you'd like any workflows updated too.

(p.s. this is awesome software, absolutely loving it.)

Tighearnán Carroll added 3 commits May 17, 2023 17:28
@robherley
Copy link
Owner

👋 I'm not forgetting about your PR, just experimenting a bit before merge.

I'm going to most likely merge this and add some additional changes after to hopefully enable a multiarch image for easy of use.

@tigh-latte
Copy link
Contributor Author

tigh-latte commented May 17, 2023

Sure thing no rush really. I've been looking into using ONBUILD and TARGETARCH to unify the docker files. If you want I can throw something together here in the next couple of hours or so that defaults to linux/amd64?

EDIT: I have something that looks to be working here now, will push shortly after I do some more testing.

FURTHER EDIT: Scratch that, having trouble cross compiling sqlite for arm64, from an amd64 machine.

@robherley
Copy link
Owner

Here's a quick wip I had going: main...robherley/arm-support-wip

Been busy so haven't really looked into it much further. I ran a docker buildx to get the multiarch amd/arm image (would definitely prefer a multiarch image over a separate tag):

docker buildx build --platform linux/arm64/v8,linux/amd64 .

Unfortunately the build is slow on my machine because of QEMU emulating the ARM portions. Also I didn't get a chance to even run the image on arm yet. The build completed but unsure if it even works on ARM 🤷

One thing I would like is to separate out the guesser code to an isolated func in a separate file, that way we can reduce the amount of duplicated code with the //go:build exclusions. If you could add that in this PR that would be great 👍

@tigh-latte
Copy link
Contributor Author

That's cool I'll fire those changes up shortly. I've also not included the CanUserGuesser function and instead just overwrite the EnabledGuesser boolean after the config is loaded from the env, just to keep using it consistent with the rest of the config. Let me know if you disagree with this.

The multiarch approach is something I'm trying too, and while I've gotten images building, they fall over when ran on my rpi because sqlite uses CGO and thus needs a gcc cross compiler to correctly create the go binary.

I've messed about with this notion a wee bit but haven't had too much success. I'm using a cross compiler based off this comment, and am now facing what looks to be an overflow issue, documented here.

Overall it's getting messy because sqlite3 using cgo. I'll have more of a play with it tomorrow.

Tighearnán Carroll added 2 commits May 18, 2023 03:04
@tigh-latte
Copy link
Contributor Author

RIGHT SO, I've gotten the multi-arch build working. Pretty happy with the result, so let me know what you think.

I've opted to use TARGETARCH instead of uname -m, so that we can build the arm64 images on any machine really (or at least, any linux/amd64 machine). To do this I've had to enable CGO on the arm64 build, and use a gcc cross compiler.

The multiarch image can be found here if you want to test it out. The last thing to do is update the build github action, I'll get on that tomorrow.

@robherley
Copy link
Owner

I've also not included the CanUserGuesser function and instead just overwrite the EnabledGuesser boolean after the config is loaded from the env

That makes a lot more sense 👍 Thanks for your dedication to getting arm working!

@robherley
Copy link
Owner

Wonder if we should consider linux/arm/v7 too 🤔

@tigh-latte
Copy link
Contributor Author

No harm in that, though I don't think I have any armv7 compatible hardware to test the sqlite cgo compilation on (I've never tried v7 on my rpi), so I won't be able to verify the result.

@robherley
Copy link
Owner

Let's roll with just arm64 for now and we can always add v7 later

@tigh-latte
Copy link
Contributor Author

Sure thing man, I'll get on the GHA later on.

@tigh-latte
Copy link
Contributor Author

Actually sorted it there now. Tested the action with act as well, so I think this is good to go if you're happy 👍

Copy link
Owner

@robherley robherley left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! 🎉 :shipit:

Curious to see how long the build action will take now that it needs to run partially in QEMU. It only runs after merge on main so shouldn't be too much friction. But definitely worth it given how many folks are using arm nowadays.

@robherley robherley merged commit f31124b into robherley:main May 18, 2023
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.

2 participants