-
Notifications
You must be signed in to change notification settings - Fork 5
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
Support MacOS along side Linux #32
Conversation
Makefile
Outdated
$(PROTOC_BIN): | ||
mkdir -p $(PROJECT_PATH)/bin | ||
$(call get-protoc,$(PROJECT_PATH)/bin,https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-$(PROTOC_VERSION)-linux-x86_64.zip,sigs.k8s.io/controller-tools/cmd/controller-gen@v0.3.0) | ||
$(call get-protoc,$(PROJECT_PATH),https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-$(PROTOC_VERSION)-$(PROTOC_OS).zip,sigs.k8s.io/controller-tools/cmd/controller-gen@v0.3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is that sigs.k8s.io/controller-tools/cmd/controller-gen@v0.3.0)
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno, didn't touch that... 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 with that
unzip -q protoc.zip bin/protoc ;\ | ||
cp bin/protoc $(1) ;\ | ||
chmod a+x $(1)/protoc ;\ | ||
unzip -q protoc.zip bin/protoc -d $(1)/. ;\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this change
diff --git a/Makefile b/Makefile
index 367fb37..7226e6f 100644
--- a/Makefile
+++ b/Makefile
@@ -68,8 +68,8 @@ stop-development:
# get-protoc will download zip from $2 and install it to $1.
define get-protoc
-@[ -f $(1) ] || { \
-echo "Downloading $(2) and installing in $(1)" ;\
+@{ \
+echo "Downloading $(2) and installing in $(1)/bin" ;\
set -e ;\
TMP_DIR=$$(mktemp -d) ;\
cd $$TMP_DIR ;\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add that, initially, as per the comment in the PR, I was also extracting the includes in $(1)/include
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally and working like a charm in Linux x86_64
I thought the includes were required too, but not extracting it seems fine too, i.e. without this: