Skip to content
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

build: update to Go 1.21.1 #4378

Merged
merged 2 commits into from
Sep 11, 2023
Merged

Conversation

uniquefine
Copy link
Contributor

@uniquefine uniquefine commented Aug 17, 2023

Update Go version to 1.21.1
Additionally, upgrade various build dependencies.


This change is Reviewable

@uniquefine uniquefine marked this pull request as ready for review August 18, 2023 13:00
@uniquefine uniquefine requested a review from a team as a code owner August 18, 2023 13:00
@uniquefine uniquefine marked this pull request as draft August 18, 2023 13:39
@uniquefine
Copy link
Contributor Author

Please don't review until I get CI to pass

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 22 files at r3.
Reviewable status: 0 of 59 files reviewed, 1 unresolved discussion (waiting on @uniquefine)


WORKSPACE line 43 at r6 (raw file):

aspect_bazel_lib_dependencies()

# Bazel rules for Golang

Reminder: drop x/sys introduced in #4385

Update the Go version to 1.21.1.
Also upgrade various build dependencies.
@oncilla oncilla changed the title go: update to v1.21.0 build: update to Go 1.21.1 Sep 7, 2023
@oncilla oncilla marked this pull request as ready for review September 7, 2023 11:33
Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 23 files at r1, 17 of 21 files at r2, 8 of 23 files at r4, 4 of 13 files at r5, 10 of 11 files at r6, 13 of 13 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @uniquefine)

a discussion (no related file):
I've attempted to build with the sqlite library toggle in .bazelrc set to sqlite_mattn.
This broke my build and it remains broken after switching back. Once I'll have fixed my normal build, I'll try to find out if this is a real issue or if it was just a freak incident.



go.mod line 52 at r7 (raw file):

	google.golang.org/grpc/examples v0.0.0-20230222033013-5353eaa44095
	// Necessary so @org_golang_google_genproto is included in the go_deps.bzl.
	google.golang.org/protobuf v1.31.0

This google.golang.org/protobuf dependency is added by go mod tidy anyways, because we use it directly in the code. I'm a bit confused what this comment is trying to say.


pkg/snet/squic/net.go line 27 at r7 (raw file):

	"time"

	quic "github.com/quic-go/quic-go"

Why the alias?

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @uniquefine)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

I've attempted to build with the sqlite library toggle in .bazelrc set to sqlite_mattn.
This broke my build and it remains broken after switching back. Once I'll have fixed my normal build, I'll try to find out if this is a real issue or if it was just a freak incident.

Nevermind, it seems to be working consistently now, must have been some strange accident.



Makefile line 33 at r7 (raw file):

go_deps.bzl: go.mod
	@# gazelle is run with "-args"; so our arguments are added to those from the gazelle() rule. 

Remove the outdated comment.

Btw. this -args is a bit odd. Luckily, everything seems to work, but it appears to have been an internal thing that we should perhaps not have used: bazel-contrib/bazel-gazelle#1546. In the old gazelle version, things did not work without the -args flag.
If I understood correctly, the original "need" for this was that some of the "args" for the gazelle commands where specified in the gazelle rule (to help doing the dance with the gotags buildflags), but some had been defined in the Makefile. Mostly things have just been moved into the BUILD file, but the update-repos thing was left in a mixed state.
We could clean this up completely , either by removing the gazelle_update_repos entirely and moving the entire call into the Makefile, or the other way around, by moving all the parameters into the gazelle_update_repos definition.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 13 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)


go.mod line 52 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

This google.golang.org/protobuf dependency is added by go mod tidy anyways, because we use it directly in the code. I'm a bit confused what this comment is trying to say.

Yeah. Also not sure. I dropped it.


Makefile line 33 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

Remove the outdated comment.

Btw. this -args is a bit odd. Luckily, everything seems to work, but it appears to have been an internal thing that we should perhaps not have used: bazel-contrib/bazel-gazelle#1546. In the old gazelle version, things did not work without the -args flag.
If I understood correctly, the original "need" for this was that some of the "args" for the gazelle commands where specified in the gazelle rule (to help doing the dance with the gotags buildflags), but some had been defined in the Makefile. Mostly things have just been moved into the BUILD file, but the update-repos thing was left in a mixed state.
We could clean this up completely , either by removing the gazelle_update_repos entirely and moving the entire call into the Makefile, or the other way around, by moving all the parameters into the gazelle_update_repos definition.

Done.


pkg/snet/squic/net.go line 27 at r7 (raw file):

Previously, matzf (Matthias Frei) wrote…

Why the alias?

Done.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @uniquefine)


Makefile line 33 at r7 (raw file):

Previously, oncilla (Dominik Roos) wrote…

Done.

💯

@oncilla oncilla merged commit 783896e into scionproto:master Sep 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants