Initial build for aarch64 (raspberry pi and Apple Silicon) #55
Conversation
Rather than pull from source and hope those crashes don't affect us, skipping upx on aarch64.
|
Hi @jpayne-procella thanks a lot for this PR, this is an important topic. A concern I have with the suggested approach is the amount of if/else overhead incurred. I wonder if we could make a dummy upx executable available on problematic architectures. I suppose we might even be able to expose an APK repository so that apk add upx installs this dummy executable. This mechanism might also scale nicely to other use cases. Is this something you had considered already, and do you have any thoughts? |
|
Yep - that was my backup approach (not as far as a repository though) :) I went this way to avoid introducing more "code", and hopefully the if/else is temporary... but if the dummy upx is the way we want to go, I can give that a crack. |
…my one, copy it over and let apk add overwrite it if it exists.
|
Hey @jpayne-procella I saw your commit land, and a cursory look says it's brilliant. I'll do some proper testing in the coming days and we can resume the PR. |
|
one more commit - i missed some cleanup in terraform.Dockerfile previously. |
There was a problem hiding this comment.
LGTM
Just tested this on my M1 Mac Mini. The build completed in 10 minutes; I wonder how much of the speedup (compared to roughly 1/2h on travis or on my Intel Mac) can be attributed to Apple silicon, and how much to just not upx'ing :)
@lukaszczerpak I don't see much point in running a dev build of this until we have multi arch build support in Travis, WDYT?
There was a problem hiding this comment.
Actually, LGTM barring the newly introduced conflict against cli.Dockerfile.
@jpayne-procella can I trouble you to fix that please?
There was a problem hiding this comment.
In general looks good! Thanks for the contribution :)
However, the way how UPX is installed produces a phantom error in the output on ARM architectures which in turn can mislead to think something went wrong. One way could be to search for the package and then install if found:
apk search --no-cache | grep -q ^upx && apk add --no-cache upx || apk add --no-cache git \
Unfortunately the original proposal as well as the above one are prone to mask a problem when UPX package changes its name, or doesn't install due to unknown reason. It will NOT be used eventually on platforms where it's supposed to be.
Maybe the following could solve the two minor issues in one shot (arch list to be defined):
apk add --no-cache $(uname -m | grep -qiE "(x86_64|i386|i686)" && echo -n upx) git \
Done |
I'm hoping that upx gets fixed for aarch64 before it changes names which means that the apk search --no-cache | grep -q ^upx approach might be better than trying to list out all architectures that should have upx - if it's supposed to install and doesn't at least we'll have an error to look at. And worst case upx doesn't get used which doesn't break functionality. The other other option I was thinking of would be adding a upx-builder image to handle the messiness. Maybe a common-builder which could pull in upx, git, curl, etc. In that case, it could even pull in upx 3.95 from alpine3.11 community repo if upx isn't in the default repo. |
The above will never stop the build if there was a problem with installation. This may result in no UPX done in case something goes wrong and we will not get any notification about this - as I outlined. So maybe combination of both ;) + no need to whitelist architectures |
done |
Agree @ynohat |
There was a problem hiding this comment.
LGTM, I'll merge and release tomorrow
@jpayne-procella would you prefer to rebase yourself, or shall I squash and merge?
Can you squash and merge please? |
This is an inelegant way of solving the lack of upx in Alpine in aarch64. If we're building for that architecture, don't upx compress the files. I considered pulling and building upx, but there's a reason it's not in Alpine.
This doesn't generate multi arch builds, it just lets someone on that architecture build and run locally. A separate PR would be needed for multiarch builds.