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.

Sign up for GitHub

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

Jump to bottom

Preliminary support for Go 1.22 #1004

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Preliminary support for Go 1.22 #1004

merged 3 commits into from
Jun 3, 2024

Conversation

eskultety
Copy link
Member

@eskultety eskultety commented May 30, 2024 •
edited by taylormadore
Loading

This PR basically just bumps the maximum supported version to 1.22 from 1.21. The changes don't include everything that 1.22 brings, most notably vendored workspaces (we don't even support base workspaces).

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • New code has type annotations
  • N/A OpenAPI schema is updated (if applicable)
  • N/A DB schema change has corresponding DB migration (if applicable)
  • README updated (if worker configuration changed, or if applicable)
  • Draft release notes are updated before merging

@eskultety
workers: tasks: gomod: Use UnsupportedFeature for Go workspaces
0b36550 
Commit c7fbbb1 framed this as an InvalidRepoStructure, but using Go
workspaces is still a perfectly valid repo structure, it's just that we
haven't added support for that feature of Go yet.

Signed-off-by: Erik Skultety <[email protected]>
@taylormadore
Copy link
Contributor

Not specifically related to this PR, but what do you think about having a log message with the version of the toolchain being executed? As I was testing these changes with a project that was using 1.22.0, I didn't see that toolchain version anywhere in the cachito request logs (just athens). It might be helpful for some future debugging to have that information available

@eskultety
Copy link
Member Author

Not specifically related to this PR, but what do you think about having a log message with the version of the toolchain being executed? As I was testing these changes with a project that was using 1.22.0, I didn't see that toolchain version anywhere in the cachito request logs (just athens). It might be helpful for some future debugging to have that information available

That version isn't explicitly reported by Go anywhere though. The only information on the target toolchain we have is go.mod so IIUC we can't really report the toolchain being executed, because from our POV that's 1.21.0 and anything newer that ends up being used by Go is totally opaque to us. If anything, I can tweak the current Using Go release: go1.21.0 message and expand it with a clause that any newer toolchain reported in go.mod will be fetched and used.

@eskultety
pkg_managers: gomod: Fix the Go toolchain selection conditions
5e63078 
When commit d9d1620 enabled full Go 1.21 support the semantics it used
for the Go toolchain selection conditions was very unfortunate as it
revolved too much around the 'go_max_version' variable which at the
time was set to be 1.21. The problem there is that once we try bumping
the max version to say 1.22 the conditions stop making sense and as a
bonus the existing tests start failing. The reasons for that are the
following:
  - we're not actually planning on updating the container image with
    each Go version we're going to support unless absolutely necessary;
    that is thanks to the GOTOOLCHAIN=auto mechanism introduced by 1.21
    that will download any new toolchain as needed automatically
  - without changing unit tests we suddenly can't pass the trivial
    condition on selecting the old fallback 1.20 if the existing unit
    test's base Go release is 1.21.X and the new max supported version
    is 1.22

Tweak the selection conditions so that they finally make sense
semantically:
- use go_max_version only to check incoming requests for incompatible
  new releases
- otherwise always explicitly compare against 1.21 and only fall back
  to 1.20 if the base release is above 1.21 which would lead to
  dirtying the source repo due to compatibility issues on Golang's side

Fixes: d9d1620

Signed-off-by: Erik Skultety <[email protected]>
@eskultety
Copy link
Member Author

Not specifically related to this PR, but what do you think about having a log message with the version of the toolchain being executed? As I was testing these changes with a project that was using 1.22.0, I didn't see that toolchain version anywhere in the cachito request logs (just athens). It might be helpful for some future debugging to have that information available

That version isn't explicitly reported by Go anywhere though. The only information on the target toolchain we have is go.mod so IIUC we can't really report the toolchain being executed, because from our POV that's 1.21.0 and anything newer that ends up being used by Go is totally opaque to us. If anything, I can tweak the current Using Go release: go1.21.0 message and expand it with a clause that any newer toolchain reported in go.mod will be fetched and used.

@taylormadore Here's my take on the log message - I added an extra commit which tweaks the Using Go release message hinting at downloading another target toolchain automatically. I'm ambivalent on whether this really improves the status quo, so I'll leave it to you whether you think it helps at all, I'm fine dropping the commit again and leave it as is.

@taylormadore
Copy link
Contributor

That version isn't explicitly reported by Go anywhere though

My initial thought was to report the output of the go version command (and making sure to use the env_vars so that the toolchain is used to report the version)

@eskultety
Copy link
Member Author

That version isn't explicitly reported by Go anywhere though

My initial thought was to report the output of the go version command (and making sure to use the env_vars so that the toolchain is used to report the version)

Oh, right. That means we have to stuff another go call in there somewhere before we commence with the module download.Okay, but that might require some bigger changes and actually not be that transparent from code perspective - because on one hand we're instantiating a specific toolchain as Go('<toolchain_string>') whereas where you want to use the bundled one (from which we report the version at the beginning) you just do it as Go(). Your suggestion falls kind of in between where it would become sort of a hybrid where you'd do go=Go(), but then instead of go.version you'd have to do go('version', env={'GOTOOLCHAIN':'auto', 'GOSUMDB':'sum.golang.org'}) just to comply with the requirement here, correct? I'm not saying it's a wrong requirement, I'm only stating that it may lead to some bigger changes rather than a simple log string, I'm only clarifying.

@weshayutin
Copy link

@brunoapimentel @taylormadore FYI several openshift teams are waiting on 1.22 support in cachito for the 4.16 OCP release :) Please review :)

@eskultety
Copy link
Member Author

@brunoapimentel @taylormadore FYI several openshift teams are waiting on 1.22 support in cachito for the 4.16 OCP release :) Please review :)

This won't likely help any of them since kubernetes uses vendored workspaces which this doesn't add support for.

@eskultety
Copy link
Member Author

That version isn't explicitly reported by Go anywhere though

My initial thought was to report the output of the go version command (and making sure to use the env_vars so that the toolchain is used to report the version)

Oh, right. That means we have to stuff another go call in there somewhere before we commence with the module download.Okay, but that might require some bigger changes and actually not be that transparent from code perspective - because on one hand we're instantiating a specific toolchain as Go('<toolchain_string>') whereas where you want to use the bundled one (from which we report the version at the beginning) you just do it as Go(). Your suggestion falls kind of in between where it would become sort of a hybrid where you'd do go=Go(), but then instead of go.version you'd have to do go('version', env={'GOTOOLCHAIN':'auto', 'GOSUMDB':'sum.golang.org'}) just to comply with the requirement here, correct? I'm not saying it's a wrong requirement, I'm only stating that it may lead to some bigger changes rather than a simple log string, I'm only clarifying.

Unless we run 'go version with the GOTOOLCHAIN=auto itself, which would comply with your request. I guess I'd be okay with that, it's just I specifically designed the toolchain download as a lazy evaluation, IOW defer downloading toolchains until the very last moment where we know we're going to download other modules rather than slow down other logic by downloading the toolchain upfront and then potentially fail anyway.

@taylormadore
Copy link
Contributor

I'd be ok with that approach and it's definitely not a blocker for this PR and we can resolve it in a follow-up issue.

@eskultety
Copy link
Member Author

I'd be ok with that approach and it's definitely not a blocker for this PR and we can resolve it in a follow-up issue.

#1005

ben-alkov
ben-alkov approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@ben-alkov ben-alkov left a comment

Choose a reason for hiding this comment

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

LGTM

@eskultety
pkg_managers: gomod: Bump the maximum supported Go version to 1.22
0d0b8b5 
This commit introduces just preliminary support of 1.22. More
specifically, we still reject any project relying on workspaces, be it
original workspaces, or vendored workspaces as the feature introduced
by 1.22.

Signed-off-by: Erik Skultety <[email protected]>
@eskultety eskultety added this pull request to the merge queue Jun 3, 2024
Merged via the queue into containerbuildsystem:master with commit fe18efa Jun 3, 2024
14 checks passed
@eskultety eskultety deleted the go122base branch June 3, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taylormadore taylormadore taylormadore approved these changes

@ben-alkov ben-alkov ben-alkov approved these changes

@brunoapimentel brunoapimentel Awaiting requested review from brunoapimentel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eskultety @taylormadore @weshayutin @ben-alkov