update protoc and related #552

huww98 · 2023-10-01T10:56:58Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

updated protoc to v25.2
updated protoc-gen-go to v1.32.0
added protoc-gen-go-grpc at v1.3.0

Modified the Makefile to use "go install" to install tools.
Use extended regular expressions in "sed" command.

Which issue(s) this PR fixes:

Fixes build on arm MacOS host
Maybe Fixing #471, not tested

Special notes for your reviewer:

Does this PR introduce an API-breaking change?:

none

huww98 · 2023-10-01T11:21:43Z

cc @jdef

huww98 · 2023-10-05T06:41:08Z

@saad-ali Fixed GitHub action. It seems we are not running protoc there, also fixed this.

spec/Makefile

Lines 46 to 48 in 80d5310

    
           # The file exists, but could be out-of-date. 
        
           $(CSI_GO): $(CSI_PROTO) 
        
           	$(MAKE) -C lib/go csi/csi.pb.go

This rule is not running because the timestamp of csi.proto is not updated.

It seems to me that we should remove all the diff in Makefile, and rely on git to check for updated file.

saad-ali · 2023-10-13T20:45:25Z

Can you make sure you've signed the CLA. See https://github.com/container-storage-interface/spec/blob/master/CONTRIBUTING.md#licence-and-cla

huww98 · 2023-11-15T07:06:32Z

@saad-ali I just sent the CCLA. Please check.

saad-ali · 2023-11-18T00:35:58Z

Confirmed, CCLA received. Thank you @huww98!

xing-yang · 2023-11-29T17:13:52Z

cc @bswartz

jdef · 2023-12-09T16:49:56Z

are we sure there are no breaking API changes here? e.g. the APIs exposed from the generated bindings seem to have changed considerably. i realize that there were no changes to the CSI API specification, but I wonder about potential breakage for folks relying on anything generated by the old compilers that may have changed. not that we want to block this PR because of that - but the potential for downstream breakage should probably be noted somewhere.

also, has anyone attempted to run these newly generated bindings against some sort of CO-maintained test suite? "CSI sanity" (or similar) comes to mind. or, at least, tried to recompile CO/CSI integration layers in conjunction w/ the changes herein?

huww98 · 2023-12-10T04:33:17Z

The only breaking I'm aware of is this:

The UnimplementedServer struct is now required to be embedded in implementations of the generated Server interface.

I will try this change on existing CSI driver and Kubernetes, and report back soon.

huww98 · 2023-12-10T08:50:57Z

@jdef See here for how I get csi-driver-host-path to work with this change. I tested this with csi-sanity. Both from master branch and recompiled with this change (I made some changes to the mock driver used in testing to compile it). The result is:

Ran 73 of 84 Specs in 0.630 seconds
SUCCESS! -- 73 Passed | 0 Failed | 1 Pending | 10 Skipped

go.mod

saad-ali

It looks like there are a lot of different changes some fixing issues, some refactoring.

Would you be willing to break the changes in to smaller (more targeted) PRs? Or would you be willing to attend one of the monthly CSI community meetings to walk us through the changes?

go.mod

saad-ali · 2024-01-31T17:22:45Z

lib/go/Makefile

@@ -21,7 +21,7 @@ export GOPATH

 # Only set PROTOC_VER if it has an empty value.
 ifeq (,$(strip $(PROTOC_VER)))
-PROTOC_VER := 3.9.1
+PROTOC_VER := 24.3


Per @bswartz where does this version number come from?

This is just the latest version from https://github.com/protocolbuffers/protobuf/releases when I open this PR. I will update it again.

huww98 · 2024-02-01T04:50:33Z

Would you be willing to break the changes in to smaller (more targeted) PRs?

It is hard. The old version does not support my arm64 MacBook. I have to upgrade the version first then tweak the Makefile to make it compile.

Or would you be willing to attend one of the monthly CSI community meetings to walk us through the changes?

OK if that is necessary.

huww98 · 2024-02-01T05:50:37Z

I've reviewed the meeting record yesterday. About the GitHub action, I've fixed it in this PRs. Please see https://github.com/huww98/csi-spec/actions/runs/7736057831/job/21092762877 .

In the action, It downloads all dependencies, run protoc, then compile the generated go file to make sure it compiles at least. Plus I have tried to integrate it into csi-driver-host-path and csi-sanity manually. I think this change should be safe enough.

lib/go/Makefile

go.mod

jdef · 2024-02-07T12:00:44Z

Will look again, was looking at each commit and didn't make it through all 3

…

On Tue, Feb 6, 2024, 11:22 PM 胡玮文 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In go.mod <#552 (comment)> : > @@ -5,12 +5,12 @@ go 1.18 require ( github.com/golang/protobuf v1.5.3 this is already fixed in the 3rd commit of this PR, please check. — Reply to this email directly, view it on GitHub <#552 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLGNW4GG6Z7QFN3ICK3YSL6OVAVCNFSM6AAAAAA5OHSWCWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNRWHA3TSNJRHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

go.mod

saad-ali · 2024-02-28T17:30:29Z

@huww98 came to the CSI meeting and walked us through the changes.

@huww98 has one more commit that may need to be added to this.

bswartz

This doesn't work on my system. The command "go env GOBIN" returns the empty string, and therefore I later get this error:

.protoc/bin/protoc -I../.. --go-grpc_out=csi --go_out=csi
--go_opt=paths=source_relative --go-grpc_opt=paths=source_relative
"csi.proto"
protoc-gen-go-grpc: program not found or is not executable
Please specify a program using absolute path or make sure the program is available in your PATH system variable
--go-grpc_out: protoc-gen-go-grpc: Plugin failed with status code 1.
make[1]: *** [Makefile:83: csi/csi.pb.go] Error 1
make[1]: Leaving directory '~/src/spec/lib/go'
make: *** [Makefile:20: lib/go/csi/csi_grpc.pb.go] Error 2

updated protoc to v25.2 updated protoc-gen-go to v1.32.0 added protoc-gen-go-grpc at v1.3.0 Modified the makefile to use "go install" to install tools. Use extended regular expressions in "sed" command. Fixes build on arm OSX host

huww98 · 2024-02-29T03:00:38Z

@bswartz fixed that.

@saad-ali Since the meeting, I've added GOBIN env support. I've also updated the second commit to avoid adding phony target then removing it, although not visible in the final codebase.

Always run "make build_go" on github actions Always update the timesptamp of spec.md/csi.proto to make sure lib/go is built on CI

- use git to check for out-of-date generated files, avoids all the temp files and manual diff in the Makefile. - not specify version for protoc-gen-go, it will use the version from go.mod - do not copy over the protoc binary. Call it directly from unzipped path. So it can find the includes automatically, and we don't need the -I or --go_opts=Mxxx flags. - use paths=source_relative to generate go files directly into destination, avoid the copy. - simplify the sed command used to extract "csi.proto" file.

bswartz

/lgtm

huww98 mentioned this pull request Oct 1, 2023

orthogonal requisite and preferred in TopologyRequirement #553

Open

huww98 force-pushed the update-protoc branch 3 times, most recently from d5566d2 to 2862f99 Compare October 5, 2023 06:10

xing-yang reviewed Jan 31, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

saad-ali reviewed Jan 31, 2024

View reviewed changes

huww98 force-pushed the update-protoc branch from f205be5 to 9373ef0 Compare February 4, 2024 03:07

jdef reviewed Feb 7, 2024

View reviewed changes

lib/go/Makefile Outdated Show resolved Hide resolved

lib/go/Makefile Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

jdef reviewed Feb 7, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

huww98 force-pushed the update-protoc branch from 9373ef0 to 0b3725b Compare February 8, 2024 11:01

huww98 force-pushed the update-protoc branch 2 times, most recently from c800de2 to b43fb50 Compare February 25, 2024 11:50

huww98 force-pushed the update-protoc branch from b43fb50 to f69defd Compare February 28, 2024 17:55

bswartz requested changes Feb 28, 2024

View reviewed changes

update protoc and related

dd3276a

updated protoc to v25.2 updated protoc-gen-go to v1.32.0 added protoc-gen-go-grpc at v1.3.0 Modified the makefile to use "go install" to install tools. Use extended regular expressions in "sed" command. Fixes build on arm OSX host

huww98 force-pushed the update-protoc branch from f69defd to 4421b74 Compare February 29, 2024 02:51

huww98 added 3 commits February 29, 2024 11:07

fix github actions

24095bc

Always run "make build_go" on github actions Always update the timesptamp of spec.md/csi.proto to make sure lib/go is built on CI

remove github.com/golang/protobuf usage

8f1e1ac

huww98 force-pushed the update-protoc branch from 4421b74 to 266ee1a Compare February 29, 2024 03:08

bswartz approved these changes Feb 29, 2024

View reviewed changes

jdef approved these changes Mar 3, 2024

View reviewed changes

jdef mentioned this pull request Mar 3, 2024

Bump google.golang.org/grpc from 1.26.0 to 1.53.0 #546

Closed

bswartz merged commit abf6346 into container-storage-interface:master Mar 5, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update protoc and related #552

update protoc and related #552

huww98 commented Oct 1, 2023 •

edited

Loading

huww98 commented Oct 1, 2023

huww98 commented Oct 5, 2023

saad-ali commented Oct 13, 2023

huww98 commented Nov 15, 2023

saad-ali commented Nov 18, 2023

xing-yang commented Nov 29, 2023

jdef commented Dec 9, 2023

huww98 commented Dec 10, 2023

huww98 commented Dec 10, 2023 •

edited

Loading

saad-ali left a comment

saad-ali Jan 31, 2024

huww98 Feb 1, 2024

huww98 commented Feb 1, 2024

huww98 commented Feb 1, 2024

jdef commented Feb 7, 2024 via email

saad-ali commented Feb 28, 2024

bswartz left a comment

huww98 commented Feb 29, 2024

bswartz left a comment

update protoc and related #552

update protoc and related #552

Conversation

huww98 commented Oct 1, 2023 • edited Loading

huww98 commented Oct 1, 2023

huww98 commented Oct 5, 2023

saad-ali commented Oct 13, 2023

huww98 commented Nov 15, 2023

saad-ali commented Nov 18, 2023

xing-yang commented Nov 29, 2023

jdef commented Dec 9, 2023

huww98 commented Dec 10, 2023

huww98 commented Dec 10, 2023 • edited Loading

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali Jan 31, 2024

Choose a reason for hiding this comment

huww98 Feb 1, 2024

Choose a reason for hiding this comment

huww98 commented Feb 1, 2024

huww98 commented Feb 1, 2024

jdef commented Feb 7, 2024 via email

saad-ali commented Feb 28, 2024

bswartz left a comment

Choose a reason for hiding this comment

huww98 commented Feb 29, 2024

bswartz left a comment

Choose a reason for hiding this comment

huww98 commented Oct 1, 2023 •

edited

Loading

huww98 commented Dec 10, 2023 •

edited

Loading