Skip to content

Add ARM support #1538

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jul 9, 2022
Merged

Add ARM support #1538

merged 15 commits into from
Jul 9, 2022

Conversation

@chadwhitacre
Copy link
Member

@chadwhitacre chadwhitacre commented Jun 22, 2022

@chadwhitacre chadwhitacre changed the title Start messing with arm Add ARM support Jun 23, 2022
@BYK
Copy link
Member

BYK commented Jun 23, 2022

Looking forward to LEG support.

@chadwhitacre
Copy link
Member Author

chadwhitacre commented Jun 23, 2022

I have to admit I googled "leg computer architecture" before remembering that you are entering into your prime with the dad jokes. 😏 😁 🤦‍♂️

@BYK
Copy link
Member

BYK commented Jun 23, 2022

@chadwhitacre this is only the beginning but I'll save Sentry's public repos of the horrors.

aminvakil
aminvakil reviewed Jun 26, 2022
View reviewed changes
Copy link
Collaborator

@aminvakil aminvakil left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So with this change there isn't any other stuff which does not work on ARM?

@chadwhitacre
Copy link
Member Author

chadwhitacre commented Jun 28, 2022

So with this change there isn't any other stuff which does not work on ARM?

Unclear. We have ARM working for main Sentry dev env so should be able to audit that ...

chadwhitacre and others added 5 commits June 29, 2022 08:29
@chadwhitacre @emmatyping
Start messing with arm
357160b
@chadwhitacre @emmatyping
Keep messing with ARM
c5315b5
@emmatyping
Configure docker platform when building
d8791f2 
This works around the issue docker/cli#3286, namely that containers are not built for linux/arm64 on Apple silicon Macs.

To solve this we add a PLATFORM variable which is used for building local containers.
@emmatyping
Fix including memory limit on ARM Clickhouse image
92c4e96
@emmatyping
Merge armmac into cwlw/arm
01745d2
emmatyping
emmatyping reviewed Jun 29, 2022
View reviewed changes
aminvakil
aminvakil reviewed Jul 2, 2022
View reviewed changes
Copy link
Collaborator

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

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

Hey @ethanhs !

emmatyping added 2 commits July 5, 2022 09:24
@emmatyping
Make platform check more robust
409485c
@emmatyping
Fix docker-compose.yml to allow running without platform environment …
e34503d 
…settings

This allows running e.g. docker-compose up -d after running ./install.sh without needing to source install/check-platform.sh
aminvakil
aminvakil approved these changes Jul 6, 2022
View reviewed changes
Copy link
Collaborator

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

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

I'm not sure if it needs more change for ARM or not, but I've tested this in a clean local machine and it works fine.

@emmatyping
Copy link
Contributor

emmatyping commented Jul 6, 2022

I'm not sure if it needs more change for ARM or not, but I've tested this in a clean local machine and it works fine.

Well, it works on my machine :) (which is an ARM/M1 Pro Mac).

I haven't tested this on ARM Linux, but it should theoretically work.

chadwhitacre
chadwhitacre commented Jul 6, 2022
View reviewed changes
chadwhitacre
chadwhitacre commented Jul 6, 2022
View reviewed changes
chadwhitacre
chadwhitacre commented Jul 6, 2022
View reviewed changes
chadwhitacre
chadwhitacre commented Jul 6, 2022
View reviewed changes
chadwhitacre
chadwhitacre commented Jul 6, 2022
View reviewed changes
@chadwhitacre
Copy link
Member Author

chadwhitacre commented Jul 6, 2022

@ethanhs lolsob I'm the PR author so I can't approve. Left some comments inline.

@emmatyping
Copy link
Contributor

emmatyping commented Jul 6, 2022

lolsob I'm the PR author so I can't approve

@chadwhitacre Ha, yeah I was going to request review from you but you can't do that on your own PR :P

emmatyping
emmatyping approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@emmatyping emmatyping left a comment

Choose a reason for hiding this comment

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

I think this is good to merge :)

@emmatyping emmatyping merged commit ddbf7cd into master Jul 9, 2022
@emmatyping emmatyping deleted the cwlw/arm branch July 9, 2022 02:03
@emmatyping emmatyping mentioned this pull request Jul 11, 2022
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@BYK BYK BYK left review comments

+2 more reviewers

@emmatyping emmatyping emmatyping approved these changes

@aminvakil aminvakil aminvakil approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chadwhitacre @BYK @emmatyping @aminvakil