-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore: unite the bin cache dir for all projects in go.work #609
chore: unite the bin cache dir for all projects in go.work #609
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
- Coverage 89.81% 88.41% -1.41%
==========================================
Files 109 111 +2
Lines 5164 5308 +144
==========================================
+ Hits 4638 4693 +55
- Misses 355 435 +80
- Partials 171 180 +9 ☔ View full report in Codecov by Sentry. |
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.
Don't forget to update
e2e/Makefile
binary cache in .github/workflows/test.yml & .github/workflows/lint.yml
controller/Makefile
Outdated
@@ -89,7 +89,7 @@ KIND ?= kind | |||
##@ Build Dependencies | |||
|
|||
## Location to install dependencies to | |||
LOCALBIN ?= $(shell pwd)/bin | |||
LOCALBIN ?= $(shell cd .. && pwd)/bin |
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 using
LOCALBIN ?= $(dir $(lastword $(MAKEFILE_LIST)))bin
in the common.mk?
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.
@Chever-John
Would you give this a try? Thanks!
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.
Sry for late~
Yes, I have tried it.
I added the following code (A) into the common.mk
file and commented out the corresponding code in the controller/Makefile
as shown below (B):
A. code
LOCALBIN ?= $(dir $(lastword $(MAKEFILE_LIST)))
$(LOCALBIN):
@mkdir -p $(LOCALBIN)
B. code
comment out the LOCALBIN ?= $(shell cd .. && pwd)/bin
## Location to install dependencies to
#LOCALBIN ?= $(shell cd .. && pwd)/bin
# Remember to remove tools downloaded into bin directory manually before updating them.
# If they need to be updated frequently, we can consider to store them in the `Dockerfile.dev`.
$(LOCALBIN):
mkdir -p $(LOCALBIN)
And I tried to run the command make sth
in the htnn/controller
dir as follows:
╭─cheverjohn at Jonathan’s MacBook Pro in ~/Workspace/OpenSource/github.com/Chever-John/htnngit:(chore/unite-the-bin-cache-dir*)
╰─± cd controller
╭─cheverjohn at Jonathan’s MacBook Pro in ~/Workspace/OpenSource/github.com/Chever-John/htnn/controllergit:(chore/unite-the-bin-cache-dir*)
╰─± make controller-gen
test -s ..//controller-gen && ..//controller-gen --version | grep -q v0.13.0 || \
GOBIN=../ go install sigs.k8s.io/controller-tools/cmd/[email protected]
cannot install, GOBIN must be an absolute path
make: *** [..//controller-gen] Error 1
Then errors came out:)
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.
How about use the following code:
# common.mk
ROOT_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST))))
LOCALBIN := $(ROOT_DIR)/bin
$(LOCALBIN):
@mkdir -p $(LOCALBIN)
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.
@Chever-John
LGTM
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.
Update!!!
from ROOT_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST))))
to ROOT_DIR := $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST))))
e2e/istio.sh
Outdated
@@ -17,7 +17,9 @@ | |||
set -eo pipefail | |||
set -x | |||
|
|||
HELM="$(pwd)/bin/helm" | |||
PARENT_DIR="$(cd .. && pwd)" |
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.
We can pass LOCALBIN to the script, like
Line 44 in e95743b
LOCALBIN=$(LOCALBIN) tools/gen-crd-code.sh |
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.
sry, I missed it here.
Please wait for me for a few minutes.
replace `LOCALBIN ?=$(cd .. && pwd)/bin` to the ```makefile ROOT_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) LOCALBIN := $(ROOT_DIR)/bin ```
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.
Here is my solution.
common.mk
Outdated
@@ -16,6 +16,11 @@ SHELL = /bin/bash | |||
OS = $(shell uname) | |||
IN_CI ?= | |||
|
|||
ROOT_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST)))) |
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.
controller/Makefile
Outdated
# Remember to remove tools downloaded into bin directory manually before updating them. | ||
# If they need to be updated frequently, we can consider to store them in the `Dockerfile.dev`. | ||
$(LOCALBIN): | ||
mkdir -p $(LOCALBIN) | ||
#$(LOCALBIN): |
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.
Just comment out the above code temporarily.
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.
Don't forget to update e2e/Makefile binary cache in .github/workflows/test.yml & .github/workflows/lint.yml
I found some code related to binary cache in the test.yml file. Is there any related code in the lint.yml file (perhaps not)?
You are right, there is no need to update lint.yml because it's already using |
Everything is set up:) |
During local testing, I encountered an issue where the local Go version is ~/Workspace/OpenSource/github.com/Chever-John/htnn/controller
☹ ../bin/controller-gen --version chore/unite-the-bin-cache-dir 53e855c
Version: v0.13.0
~/Workspace/OpenSource/github.com/Chever-John/htnn/controller
☹ go version chore/unite-the-bin-cache-dir 53e855c
go version go1.22.1 darwin/arm64
~/Workspace/OpenSource/github.com/Chever-John/htnn/controller
☺ make manifests chore/unite-the-bin-cache-dir 53e855c
/Users/cheverjohn/Workspace/OpenSource/github.com/Chever-John/htnn/bin/controller-gen rbac:roleName=htnn-role crd:ignoreUnexportedFields=true \
output:crd:artifacts:config=../manifests/charts/htnn-controller/templates/crds/ \
output:rbac:artifacts:config=../manifests/charts/htnn-controller/templates/rbac/ \
paths="../types/..." paths="./internal/controller/..."
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102a1c604]
goroutine 182 [running]:
go/types.(*Checker).handleBailout(0x14000c27800, 0x14001811d18)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/check.go:367 +0x9c
panic({0x102cf5a60?, 0x103294af0?})
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/runtime/panic.go:770 +0x124
go/types.(*StdSizes).Sizeof(0x0, {0x102dc0990, 0x10329d300})
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/sizes.go:228 +0x314
go/types.(*Config).sizeof(...)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/sizes.go:333
go/types.representableConst.func1({0x102dc0990?, 0x10329d300?})
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/const.go:76 +0x9c
go/types.representableConst({0x102dc6c30, 0x103268240}, 0x14000c27800, 0x10329d300, 0x14001811468)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/const.go:92 +0x138
go/types.(*Checker).representation(0x14000c27800, 0x14001814d00, 0x10329d300)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/const.go:256 +0x68
go/types.(*Checker).implicitTypeAndValue(0x14000c27800, 0x14001814d00, {0x102dc09b8, 0x140002342a0})
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/expr.go:375 +0x304
go/types.(*Checker).assignment(0x14000c27800, 0x14001814d00, {0x102dc09b8, 0x140002342a0}, {0x102b34081, 0x14})
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/assignments.go:52 +0x23c
go/types.(*Checker).initConst(0x14000c27800, 0x14001dbf500, 0x14001814d00)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/assignments.go:126 +0x274
go/types.(*Checker).constDecl(0x14000c27800, 0x14001dbf500, {0x102dc3538, 0x1400132a1a0}, {0x102dc3538, 0x1400132a1c0}, 0x0)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/decl.go:490 +0x250
go/types.(*Checker).objDecl(0x14000c27800, {0x102dcbd20, 0x14001dbf500}, 0x0)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/decl.go:191 +0x84c
go/types.(*Checker).packageObjects(0x14000c27800)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/resolver.go:693 +0x468
go/types.(*Checker).checkFiles(0x14000c27800, {0x14001180000, 0x5, 0x5})
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/check.go:408 +0x164
go/types.(*Checker).Files(...)
/Users/cheverjohn/Infra/dev-env/langs/go/go1.22.1/src/go/types/check.go:372
sigs.k8s.io/controller-tools/pkg/loader.(*loader).typeCheck(0x1400025f4d0, 0x14001090fc0)
/Users/cheverjohn/Workspace/golang/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:286 +0x2d8
sigs.k8s.io/controller-tools/pkg/loader.(*Package).NeedTypesInfo(0x14001090fc0)
/Users/cheverjohn/Workspace/golang/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:99 +0x44
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check(0x140017a7290, 0x14001090fc0)
/Users/cheverjohn/Workspace/golang/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:268 +0x304
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check.func1(0x5b?)
/Users/cheverjohn/Workspace/golang/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:262 +0x58
created by sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check in goroutine 201
/Users/cheverjohn/Workspace/golang/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:260 +0x230
make: *** [manifests] Error 2 This problem can be stably reproduced. Related discussion as following issues: kubernetes-sigs/controller-tools#888 Solution: change I don't know how to fix this problem in which way. Should I create a new issue to fix it or just solve it in this PR? |
Let's fix it in the new PR, Thanks! |
So I'm curious, what kind of solution should I choose? Do you choose to upgrade the version of |
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.
Yes, let's update the version |
6a571fd
to
6d5c547
Compare
site/Makefile
Outdated
# If they need to be updated frequently, we can consider to store them in the `Dockerfile.dev`. | ||
$(LOCALBIN): | ||
@mkdir -p $(LOCALBIN) | ||
SHELL = /bin/bash |
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.
The common.mk already provides SHELL?
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.
Yes!
Have removed it!
Merged. Thanks! |
Fix #426
I am working on issue #426.
I hope that my work is on the right track. I have thoroughly run every makefile command related to the project to ensure everything is functioning correctly.
Looking forward to your feedback and hoping my contribution is valuable.
Thank you!