You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/05/17 11:05:54 UTC
[GitHub] [yunikorn-scheduler-interface] craigcondit commented on a diff in pull request #66: [YUNIKORN-1209] SI build failed on arm64
craigcondit commented on code in PR #66:
URL: https://github.com/apache/yunikorn-scheduler-interface/pull/66#discussion_r874679030
##########
lib/go/Makefile:
##########
@@ -51,6 +52,8 @@ PROTOGEN_ARCH := amd64
ifeq (i386,$(PROTOC_ARCH))
PROTOC_ARCH := x86_32
PROTOGEN_ARCH := 386
+else ifeq (arm64, $(PROTOC_ARCH))
+PROTOC_ARCH := x86_64
Review Comment:
Let's fix this properly. Hacking arm64 -> x86_64 just breaks arm builds on Linux. protoc 3.20.1 and later support arm64 on Mac, and should be backwards compatible.
Some other changes we need WRT versioning:
PROTOC_OS and PROTOC_ARCH need some massaging, as the releases don't match exactly what `uname -a` / `uname -s` return. Can we do something lik:
```
-# Fix OS string for Mac builds.
+# Fix OS string for Mac and Linux builds.
PROTOC_OS := $(shell uname -s)
PROTOGEN_OS := $(shell uname -s)
ifeq (Darwin,$(PROTOC_OS))
PROTOC_OS := osx
endif
+ifeq (Linux,$(PROTOC_OS))
+PROTOC_OS := linux
+endif
-# Allow building on 32 bit machines.
+# Fix architecture naming.
PROTOC_ARCH := $(shell uname -m)
PROTOGEN_ARCH := amd64
ifeq (i386,$(PROTOC_ARCH))
PROTOC_ARCH := x86_32
PROTOGEN_ARCH := 386
else ifeq (arm64, $(PROTOC_ARCH))
-PROTOC_ARCH := x86_64
+PROTOC_ARCH := aarch_64
+else ifeq (aarch64, $(PROTOC_ARCH))
+PROTOC_ARCH := aarch_64
endif
```
##########
lib/go/Makefile:
##########
@@ -51,6 +52,8 @@ PROTOGEN_ARCH := amd64
ifeq (i386,$(PROTOC_ARCH))
PROTOC_ARCH := x86_32
PROTOGEN_ARCH := 386
+else ifeq (arm64, $(PROTOC_ARCH))
+PROTOC_ARCH := x86_64
Review Comment:
Let's fix this properly. Hacking arm64 -> x86_64 just breaks arm builds on Linux. protoc 3.20.1 and later support arm64 on Mac, and should be backwards compatible.
Some other changes we need WRT versioning:
PROTOC_OS and PROTOC_ARCH need some massaging, as the releases don't match exactly what `uname -a` / `uname -s` return. Can we do something like:
```
-# Fix OS string for Mac builds.
+# Fix OS string for Mac and Linux builds.
PROTOC_OS := $(shell uname -s)
PROTOGEN_OS := $(shell uname -s)
ifeq (Darwin,$(PROTOC_OS))
PROTOC_OS := osx
endif
+ifeq (Linux,$(PROTOC_OS))
+PROTOC_OS := linux
+endif
-# Allow building on 32 bit machines.
+# Fix architecture naming.
PROTOC_ARCH := $(shell uname -m)
PROTOGEN_ARCH := amd64
ifeq (i386,$(PROTOC_ARCH))
PROTOC_ARCH := x86_32
PROTOGEN_ARCH := 386
else ifeq (arm64, $(PROTOC_ARCH))
-PROTOC_ARCH := x86_64
+PROTOC_ARCH := aarch_64
+else ifeq (aarch64, $(PROTOC_ARCH))
+PROTOC_ARCH := aarch_64
endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org