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 #10258 related to segmentaiton fault on mac m1 arm64 #10279

Merged
merged 5 commits into from
Oct 20, 2021

Conversation

9M6
Copy link
Contributor

@9M6 9M6 commented Oct 1, 2021

Description

Closes: #10258

fixed an issue where the makefile target build-linux would build a faulty binary file on a mac m1, since the compilation target
was AMD64, but it was needed to be ARM64.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@9M6 9M6 force-pushed the fix-makefile-build-linux-for-arm64-mac-m1 branch from dad2017 to 2431883 Compare October 1, 2021 09:42
@amaury1093 amaury1093 self-assigned this Oct 4, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

utACK (just tested on my non-M1 mac, it builds).

Maybe someone with a M1 can test it before merging? cc @frumioj

@9M6
Copy link
Contributor Author

9M6 commented Oct 5, 2021

utACK (just tested on my non-M1 mac, it builds).

Maybe someone with a M1 can test it before merging? cc @frumioj
@AmauryM

Just a heads-up, this PR fixes only the build-linux make target, there's another follow up issue
with the make target localnet-start which for some reason if the image is built as arm64 it wont run.

The solution that I have written works well with the issue above, since when running the localnet-start the image will be built on amd64 arch.

GOARCH=$(if $(findstring aarch64,$(shell uname -m)),arm64,amd64) the function will return ARM64 in case it finds aarch64 which is the case for alpine, but if the build target is run on a M1 it will return AMD64, since MacOS outputs by default arm64 and the make function does not find aarch64.

Copy link
Contributor

@frumioj frumioj left a comment

Choose a reason for hiding this comment

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

If the suggested change is committed, LGTM. Pre-approving.

Makefile Outdated
@@ -109,7 +109,7 @@ BUILD_TARGETS := build install

build: BUILD_ARGS=-o $(BUILDDIR)/
build-linux:
GOOS=linux GOARCH=amd64 LEDGER_ENABLED=false $(MAKE) build
GOOS=linux GOARCH=$(if $(findstring aarch64,$(shell uname -m)),arm64,amd64) LEDGER_ENABLED=false $(MAKE) build
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GOOS=linux GOARCH=$(if $(findstring aarch64,$(shell uname -m)),arm64,amd64) LEDGER_ENABLED=false $(MAKE) build
GOOS=linux GOARCH=$(if $(findstring aarch64,$(shell uname -m)) || $(findstring arm64, $(shell uname -m)),arm64,amd64) LEDGER_ENABLED=false $(MAKE) build

Copy link
Contributor

Choose a reason for hiding this comment

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

Because uname -m reports differently on Debian/M1 vs. MacOS/M1:

johnk@debian:~/src/cosmos-sdk$ uname -m
aarch64

MacOS/M1

frumiousj@dirtycomputer cosmos-sdk % uname -m
arm64
frumiousj@dirtycomputer cosmos-sdk % uname
Darwin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not work well with localnet-start as I've explained in the comment above, for some reason, in MacOS/M1, when the build GOARCH is set to ARM64, the localnet-start binary (when executed) will output that there's a architecture mismatch.

@frumioj
Copy link
Contributor

frumioj commented Oct 5, 2021

utACK (just tested on my non-M1 mac, it builds).
Maybe someone with a M1 can test it before merging? cc @frumioj
@AmauryM

Just a heads-up, this PR fixes only the build-linux make target, there's another follow up issue with the make target localnet-start which for some reason if the image is built as arm64 it wont run.

It won't run because of incompatibility between the image architecture and that of the docker image it's being run in. I did actually find a way to fix that some time ago IIRC, but my branch is still a WIP: regen-network/regen-ledger#410 - perhaps that might be helpful though?

@9M6
Copy link
Contributor Author

9M6 commented Oct 7, 2021

utACK (just tested on my non-M1 mac, it builds).
Maybe someone with a M1 can test it before merging? cc @frumioj
@AmauryM

Just a heads-up, this PR fixes only the build-linux make target, there's another follow up issue with the make target localnet-start which for some reason if the image is built as arm64 it wont run.

It won't run because of incompatibility between the image architecture and that of the docker image it's being run in. I did actually find a way to fix that some time ago IIRC, but my branch is still a WIP: regen-network/regen-ledger#410 - perhaps that might be helpful though?

Will check it out, thank you 😄

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 13, 2021
@tac0turtle tac0turtle removed the A:automerge Automatically merge PR once all prerequisites pass. label Oct 13, 2021
@tac0turtle
Copy link
Member

what is the status here?

@amaury1093
Copy link
Contributor

Ping. Is it okay to put automerge? We have 2 approvals

@9M6
Copy link
Contributor Author

9M6 commented Oct 19, 2021

Ping. Is it okay to put automerge? We have 2 approvals

I will check if the suggested change from @frumioj breaks anything, since as he pointed out this fix might not work with Debian/M1.

@9M6
Copy link
Contributor Author

9M6 commented Oct 20, 2021

@AmauryM I've tested the changes suggested by @frumioj and its working well on my side.

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 20, 2021
@mergify mergify bot merged commit cdd95ba into cosmos:master Oct 20, 2021
@tac0turtle
Copy link
Member

running make-simd-linux fails for 64 Segmentation fault go mod download.

This also seems to have broken ci livness test

@tac0turtle tac0turtle mentioned this pull request Nov 15, 2021
4 tasks
@9M6
Copy link
Contributor Author

9M6 commented Nov 15, 2021

running make-simd-linux fails for 64 Segmentation fault go mod download.

This also seems to have broken ci livness test

What Arch?

@tac0turtle
Copy link
Member

this is within the docker container. when you run make build-simd-linux it fails

@9M6
Copy link
Contributor Author

9M6 commented Nov 15, 2021

this is within the docker container. when you run make build-simd-linux it fails

Can you tell me how to replicate?

@webmaster128
Copy link
Member

If I am not missing something, this PR introduced a regression (see #12798 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation fault when running (make) localnet-start on linux/arm64 (Apple M1 Chip)
5 participants