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

x/telemetry: temporarily downgrade go requirement to go1.21 #70056

Closed
hyangah opened this issue Oct 25, 2024 · 8 comments
Closed

x/telemetry: temporarily downgrade go requirement to go1.21 #70056

hyangah opened this issue Oct 25, 2024 · 8 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. telemetry x/telemetry issues
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Oct 25, 2024

Adding golang.org/x/telemetry to Delve's dependency (go-delve/delve#3815)
resulted in Go version requirement change in Delve's go.mod because golang.org/x/telemetry@latest
requires go1.22.0. Delve had been operating with a wider version support policy (3 latest go versions) for a while.
So this needs extra discussion in Delve community.

We need CL 616396 but that is past x/telemetry/go.mod go version update.
Can we temporarily revert go.mod version update change so delve can pick up?
(CL 622735)

Alternatively, can we defer the version bump in x/telemetry until it's really needed?
Looks like x/sys and x/sync are left with older go versions.
We can also consider removing dependency on x/mod.

cc @golang/telemetry

@gopherbot gopherbot added the telemetry x/telemetry issues label Oct 25, 2024
@gopherbot gopherbot added this to the Unreleased milestone Oct 25, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@adonovan
Copy link
Member

adonovan commented Oct 25, 2024

Unfortunately that won't be so simple because the x/tools repo has changed a lot recently under the assumption that go1.22 is the minimum version. It might be easier to prepare a branch of x/telemetry that has whatever delve needs and for delve's go.mod file to use a replace directive that names this branch.

@findleyr
Copy link
Member

@adonovan I don't follow. x/telemetry doesn't require x/tools -- only x/sync and x/sys, both of which are on 1.18.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/622735 mentions this issue: go.mod: temporarily downgrade go requirement to go1.21

@hyangah
Copy link
Contributor Author

hyangah commented Oct 28, 2024

@adonovan x/telemetry does not have dependency on x/tools. I'd say, x/telemetry should have minimal dependencies. Also, if we want to use Go telemetry to monitor the usage of Go versions, I think it makes more sense to avoid requiring latest go versions to avoid selection bias.

If you are concerned about Delve's existing dependency on x/tools, Delve does not need the latest version of x/tools module. The dependencies are mostly coming from their code generation scripts or tests. Unfortunately, Go supports only one build graph and mixes tools/tests dependencies. Otherwise, there is no reason that delve should require the latest x/tools that moves the go version requirements as well.

@hyangah
Copy link
Contributor Author

hyangah commented Oct 28, 2024

@findleyr BTW, I also noticed that not having a tag prevents the Go command from picking the latest version of x/telemetry module that supports Go 1.N correctly. Should we start tagging x/telemetry, too?

gopherbot pushed a commit to golang/telemetry that referenced this issue Oct 28, 2024
For golang/go#70056

Change-Id: I927cd60b13f8bee87b2df675f6637f3b30df81cd
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/622735
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
hyangah added a commit to hyangah/delve that referenced this issue Oct 28, 2024
Imported golang.org/x/telemetry@9c0d19e to avoid
go version requirement change.

For go-delve#3815
For golang/go#70056
@adonovan
Copy link
Member

I don't follow. x/telemetry doesn't require x/tools -- only x/sync and x/sys, both of which are on 1.18.

Er, quite right, never mind me.

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 28, 2024
derekparker pushed a commit to go-delve/delve that referenced this issue Oct 29, 2024
Imported golang.org/x/telemetry@9c0d19e to avoid
go version requirement change.

For #3815
For golang/go#70056
@hyangah
Copy link
Contributor Author

hyangah commented Nov 7, 2024

Change in delve was submitted.
CL 626357 will change the go version back to 1.22.0.

@hyangah hyangah closed this as completed Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. telemetry x/telemetry issues
Projects
None yet
Development

No branches or pull requests

6 participants