Avoid redownloading all dependencies (protoc, gogo) #337

leeavital · 2024-08-29T19:51:17Z

What does this PR do?

Adds a rake setup_gogo task which will clone gogo into a new toolchains directory (which is git ignored).

On every run of codegen, we'll checkout a pinned version of gogo and rebuild it. This should be pretty fast as long as gogo is already checked out.

This PR also adds a new 'toolchains' location where all dependencies are cached. Before we were jumbling a bunch of dependencies into /tmp.

This led to a weird case where we were accidentally (?) using a new protobuf compiler with a legacy .proto library (for stuff like google.Duration). This is now explicit.

The structure of the new toolchains is described in the Rakefile

Reviewer's Checklist

Reviewers: please see the review guidelines.

sgnn7

@leeavital Left a few comments

Rakefile

.gitignore

Rakefile

sgnn7

@leeavital Left some comments

Rakefile

sgnn7 · 2024-09-03T22:14:19Z

Rakefile

-        fi
+
+          mkdir -p #{toolchain_include_dir}
+          echo "here"


Suggested change

echo "here"

sgnn7 · 2024-09-03T22:15:38Z

Rakefile


      echo "Generating process proto (go)"
-      PATH=/tmp/gogo-bin-v1.3.2 #{protoc_legacy_binary} --proto_path=$GOPATH/src:#{gogo_dir}/src:. --gogofaster_out=$GOPATH/src proto/process/*.proto
+      PATH=#{toolchain_bin_dir} #{protoc_binary} --proto_path=#{toolchain_legacy_include_dir}:#{gogo_include}:. --gogofaster_out=$GOPATH/src proto/process/*.proto


Is it intended to change from protoc_legacy_binary to protoc_binary here?

It was not, but I think since CI is verifying that the generated code isn't changing it's fine to upgrade (that's an expected outcome since both should just call the gogofaster binary )

Hmm - I'm a bit afraid of unintended changes to be honest of which I'm not fully sure of in the broader scope. Maybe this can be split out into a separate PR to ensure 1:1 functionality in this one?

sgnn7 · 2024-09-03T22:16:10Z

Rakefile

+      GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0
+      GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0


Suggested change

GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0

GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0

GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0

sgnn7 · 2024-09-03T22:17:12Z

Rakefile

-      PATH=#{protoc_gen_go_dir}/bin #{protoc_legacy_binary} --proto_path=$GOPATH/src:#{gogo_dir}/src:.  --gostreamer_out=$GOPATH/src proto/process/*.proto
+      GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0
+      GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0
+      PATH=#{toolchain_bin_dir} #{protoc_binary} --proto_path=$GOPATH/src:#{gogo_dir}/src:.  --gostreamer_out=$GOPATH/src proto/process/*.proto


Is it intended to change from protoc_legacy_binary to protoc_binary here?

Same answer as above

sgnn7 · 2024-09-03T22:19:08Z

Rakefile


      echo "Generating contlcycle proto"
-      PATH=#{protoc_gen_go_dir}/bin #{protoc_binary} --proto_path=$GOPATH/src:. --go_out=$GOPATH/src proto/contlcycle/contlcycle.proto
+      PATH=#{toolchain_bin_dir} #{protoc_binary} --proto_path=#{toolchain_legacy_include_dir}:. --go_out=$GOPATH/src proto/contlcycle/contlcycle.proto


Should the proto path be toolchain_legacy_include_dir or toolchain_include_dir?

PS: I see a number of these occurrences below too

It's actually supposed to be the legacy include directory. I was suprised too and it took me a bit of digging to discovery this.

If you use the new include directory, then the go types will be subtly different (i.e. use google.golang.org/protobuf/types instead of the deprecated github.com/golang/protobuf/ptypes). This is probably fine, but technically a break that should be tested separately, so I'm keeping the old behavior in this PR

leeavital · 2024-09-04T13:26:00Z

@sgnn7 this should be ready for another review

sgnn7

Just a note that we may want to split the conversion from old to new protobuf in some lines to a separate PR but it shouldn't be a blocker I think.

Adds a `rake setup_gogo` task which will clone gogo into a new toolchains directory (which is git ignored). On every run of codegen, we'll checkout a pinned version of gogo and rebuild it. This should be pretty fast as long as gogo is already checked out. This PR also adds a new 'toolchains' location where all dependencies are cached. Before we were jumbling a bunch of dependencies into /tmp. This led to a weird case where we were accidentally (?) using a new protobuf compiler with a legacy .proto library (for stuff like google.Duration). This is now explicit. The structure of the new toolchains is described in the Rakefile

leeavital · 2024-09-05T21:41:20Z

/merge

dd-devflow · 2024-09-05T21:41:26Z

🚂 MergeQueue: pull request added to the queue

The median merge time in master is 2m.

Use /merge -c to cancel this operation!

In #337 we discovered that CWS payloads were being generated with a new protobuf compiler but an older set of common type definitions. This change updates the generated code to be generated with new common type definitions. The old generated code is not a problem today, but might become an issue if we need to update the protobuf stack for some reason in the future. Per the [version support policy](https://protobuf.dev/support/version-support/#changes), the wireformat will not change, however the generated code can (and does, in this PR) change which might make integrating these changes hard. The main difference is that common types like 'timestamp' now use go types like 'google.golang.org/protobuf/types/known/timestamppb instead of github.com/golang/protobuf/ptypes/timestamp.

This updates contlcycle, autoscaling, and contimage to generate their payloads using non-deprecated shared protobuf definitions. This will help us reduce some cruft in the build script. The changes are wire compatible, but the generated go code might not compile cleanly into the agent/backend. See for context * #337 * #342

As described in #337 we used to use two different protobuf compilers in this repo. Now we only use one, so we can get rid of references to the old one.

leeavital requested a review from a team as a code owner August 29, 2024 19:51

leeavital force-pushed the push-rruqxklnzryn branch from 967aa09 to 9ac0051 Compare August 29, 2024 19:57

leeavital marked this pull request as draft August 29, 2024 19:58

leeavital force-pushed the push-rruqxklnzryn branch 4 times, most recently from a6b0bca to ecba6e7 Compare August 29, 2024 20:21

sgnn7 requested changes Aug 29, 2024

View reviewed changes

Rakefile Outdated Show resolved Hide resolved

Rakefile Show resolved Hide resolved

.gitignore Show resolved Hide resolved

Rakefile Outdated Show resolved Hide resolved

leeavital force-pushed the push-rruqxklnzryn branch 5 times, most recently from 2275071 to cb1c8de Compare August 31, 2024 21:17

leeavital mentioned this pull request Sep 3, 2024

Adjust constants in rakefile #339

Merged

leeavital changed the base branch from master to lee.avital/refactor_rakefile September 3, 2024 12:05

leeavital force-pushed the push-rruqxklnzryn branch 6 times, most recently from a189dd3 to b2c1e8e Compare September 3, 2024 12:43

leeavital changed the title ~~Avoid re-downloading and rebuilding gogo every time we run codegen~~ Avoid redownloading all dependencies (protoc, gogo) Sep 3, 2024

leeavital force-pushed the push-rruqxklnzryn branch 2 times, most recently from 7288c0a to 0bc0e1a Compare September 3, 2024 13:18

leeavital marked this pull request as ready for review September 3, 2024 13:20

leeavital force-pushed the push-rruqxklnzryn branch from 0bc0e1a to 628bfc1 Compare September 3, 2024 13:32

sgnn7 requested changes Sep 3, 2024

View reviewed changes

leeavital force-pushed the lee.avital/refactor_rakefile branch from 00b3937 to 9dc090c Compare September 3, 2024 22:51

leeavital force-pushed the push-rruqxklnzryn branch 2 times, most recently from 2e1c636 to 43bbb15 Compare September 3, 2024 23:08

leeavital changed the base branch from lee.avital/refactor_rakefile to master September 3, 2024 23:09

leeavital force-pushed the push-rruqxklnzryn branch from 43bbb15 to 2bd7953 Compare September 4, 2024 18:59

leeavital requested a review from sgnn7 September 4, 2024 19:47

sgnn7 approved these changes Sep 5, 2024

View reviewed changes

leeavital force-pushed the push-rruqxklnzryn branch from 2bd7953 to 107b12d Compare September 5, 2024 21:38

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: queued labels Sep 5, 2024

dd-mergequeue bot merged commit acc1258 into master Sep 5, 2024
8 checks passed

dd-devflow bot added mergequeue-status: done and removed mergequeue-status: in_progress labels Sep 5, 2024

leeavital mentioned this pull request Sep 6, 2024

Update logs java protobuf #340

Closed

leeavital mentioned this pull request Sep 6, 2024

Update CWS payloads off deprecated protobuf types #342

Merged

leeavital mentioned this pull request Sep 10, 2024

Update container-app to use newer include files #344

Merged

leeavital mentioned this pull request Sep 25, 2024

[protobuf] remove legacy protobuf compiler #347

Merged

leeavital added a commit that referenced this pull request Oct 1, 2024

[protobuf] remove legacy protobuf compiler

6b35fef

As described in #337 we used to use two different protobuf compilers in this repo. Now we only use one, so we can get rid of references to the old one.

dd-mergequeue bot pushed a commit that referenced this pull request Oct 1, 2024

[protobuf] remove legacy protobuf compiler (#347)

08cc9ec

As described in #337 we used to use two different protobuf compilers in this repo. Now we only use one, so we can get rid of references to the old one.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid redownloading all dependencies (protoc, gogo) #337

Avoid redownloading all dependencies (protoc, gogo) #337

leeavital commented Aug 29, 2024 •

edited

Loading

sgnn7 left a comment

sgnn7 left a comment

sgnn7 Sep 3, 2024

sgnn7 Sep 3, 2024

leeavital Sep 3, 2024

sgnn7 Sep 5, 2024

sgnn7 Sep 3, 2024

sgnn7 Sep 3, 2024

leeavital Sep 3, 2024

sgnn7 Sep 3, 2024

leeavital Sep 3, 2024

leeavital commented Sep 4, 2024

sgnn7 left a comment

leeavital commented Sep 5, 2024

dd-devflow bot commented Sep 5, 2024

		GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0
		GOPATH=#{toolchain_dir} go install github.com/leeavital/protoc-gen-gostreamer@v0.1.0

Avoid redownloading all dependencies (protoc, gogo) #337

Avoid redownloading all dependencies (protoc, gogo) #337

Conversation

leeavital commented Aug 29, 2024 • edited Loading

What does this PR do?

Reviewer's Checklist

sgnn7 left a comment

Choose a reason for hiding this comment

sgnn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeavital commented Sep 4, 2024

sgnn7 left a comment

Choose a reason for hiding this comment

leeavital commented Sep 5, 2024

dd-devflow bot commented Sep 5, 2024

leeavital commented Aug 29, 2024 •

edited

Loading