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/16 06:03:30 UTC
[GitHub] [yunikorn-scheduler-interface] manirajv06 opened a new pull request, #66: [YUNIKORN-1209] SI build failed on arm64
manirajv06 opened a new pull request, #66:
URL: https://github.com/apache/yunikorn-scheduler-interface/pull/66
### What is this PR for?
In addition to fixing build issues on Mac M1, also modified the way in how other related binaries protoc-gen-go & protoc-gen-go-grpc has been pulled and used as part of building SI.
### What type of PR is it?
* [ ] - Bug Fix
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1209
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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
[GitHub] [yunikorn-scheduler-interface] craigcondit commented on a diff in pull request #66: [YUNIKORN-1209] SI build failed on arm64
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [yunikorn-scheduler-interface] craigcondit commented on a diff in pull request #66: [YUNIKORN-1209] SI build failed on arm64
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #66:
URL: https://github.com/apache/yunikorn-scheduler-interface/pull/66#discussion_r874902106
##########
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:
protoc 3.20.1 produces identical Go output to 3.16.0 so there's no issue in using the newer version. I've verified this locally on linux/aarch_64. This doesn't change any runtime or compile-time dependencies for our code. Let's update the version to 3.20.1.
--
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
[GitHub] [yunikorn-scheduler-interface] manirajv06 commented on a diff in pull request #66: [YUNIKORN-1209] SI build failed on arm64
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on code in PR #66:
URL: https://github.com/apache/yunikorn-scheduler-interface/pull/66#discussion_r874869690
##########
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:
> protoc 3.20.1 and later support arm64 on Mac, and should be backwards compatible.
Agreed but unfortunately corresponding version url for v3.16.0
(https://github.com/protocolbuffers/protobuf/releases/download/v3.16.0/) used to download has only protoc-3.16.0-osx-x86_64.zip not as we are expecting protoc-3.16.0-osx-aarch_64.zip. Where as, v3.20.1 releases has both zip for osx. Hence keeping x86_64 for protoc download. However, we should fix this in a generic way later as already there are open issues around version, grpc plugins etc as discussed in https://issues.apache.org/jira/browse/YUNIKORN-815
--
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
[GitHub] [yunikorn-scheduler-interface] craigcondit closed pull request #66: [YUNIKORN-1209] SI build failed on arm64
Posted by GitBox <gi...@apache.org>.
craigcondit closed pull request #66: [YUNIKORN-1209] SI build failed on arm64
URL: https://github.com/apache/yunikorn-scheduler-interface/pull/66
--
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