Skip to content

Build with Go 1.18 #6605

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
Merged
AkihiroSuda merged 2 commits into from
Mar 19, 2022
Merged

Build with Go 1.18 #6605

AkihiroSuda merged 2 commits into from
Mar 19, 2022

Conversation

@kzys
Copy link
Member

@kzys kzys commented Mar 2, 2022 •
edited
Loading

The first commit switches from Go 1.17 to Go 1.18. The second commit revendor dependencies with Go 1.17's module pruning.

@k8s-ci-robot
Copy link

k8s-ci-robot commented Mar 2, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 2, 2022

Build succeeded.

  • containerd-build-arm64 : RETRY_LIMIT in 3m 53s (non-voting)
  • containerd-test-arm64 : RETRY_LIMIT in 6m 24s (non-voting)
  • containerd-integration-test-arm64 : RETRY_LIMIT in 28m 52s (non-voting)

@zhsj
Copy link
Contributor

zhsj commented Mar 3, 2022

I wonder if you have met the error in #6586

@kzys
Copy link
Member Author

kzys commented Mar 3, 2022

I wonder if you have met the error in #6586

Yeah. I wanted to know what we really need and how does that work with Go 1.16.

@kzys kzys force-pushed the go-118 branch 3 times, most recently from b8b616d to c3ff7d9 Compare March 3, 2022 17:42
@kzys
Copy link
Member Author

kzys commented Mar 3, 2022

@thaJeztah I don't fully understand the motivation of #5441. After removing the empty-mod hack, seems go mod vendor doesn't bring outdated dependencies but go.sum has these.

@thaJeztah
Copy link
Member

thaJeztah commented Mar 3, 2022

It's possible that go 1.17 improved some things (or that dependencies were updated) at the time, without it, some dependencies were resolved, causing other dependencies to be updated (or introduced)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 3, 2022

Build succeeded.

  • containerd-build-arm64 : RETRY_LIMIT in 3m 45s (non-voting)
  • containerd-test-arm64 : RETRY_LIMIT in 6m 05s (non-voting)
  • containerd-integration-test-arm64 : RETRY_LIMIT in 25m 08s (non-voting)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 3, 2022

Build succeeded.

  • containerd-build-arm64 : RETRY_LIMIT in 4m 41s (non-voting)
  • containerd-test-arm64 : RETRY_LIMIT in 6m 32s (non-voting)
  • containerd-integration-test-arm64 : RETRY_LIMIT in 30m 45s (non-voting)

@kzys
Copy link
Member Author

kzys commented Mar 3, 2022

It's possible that go 1.17 improved some things (or that dependencies were updated) at the time, without it, some dependencies were resolved, causing other dependencies to be updated (or introduced)

Hmm, so, do we care newly-added outdated dependencies in go.sum? I still don't quite understand the side-effect of them, if there is.

@thaJeztah
Copy link
Member

thaJeztah commented Mar 3, 2022

I don't think the extra ones in go.sum are a huge deal, other than potentially causing more conflicts between PRs. The original issue actually affected a dependency (which, from your comment, and from golang/go#51285 (comment) should no longer seems to be a problem).

It's a bit infuriating though that Go keeps on introducing breaking changes in modules, and not consider them "breaking" because "tooling is out of scope for the Go 1.x stability guarantee" ... sigh

@kzys
Copy link
Member Author

kzys commented Mar 3, 2022

Good. Then I'm going to remove the empty-mod directory here and keep this PR till Go 1.18's official release.

@estesp estesp mentioned this pull request Mar 9, 2022
Closed
@mxpv
Copy link
Member

mxpv commented Mar 15, 2022

Meantime Go 1.18 has been released: https://go.dev/dl/
Worth to either update/rebase this PR or open a new one.

@kzys kzys changed the title Build with Go 1.18 RC1 Build with Go 1.18 Mar 16, 2022
@kzys kzys marked this pull request as ready for review March 16, 2022 17:12
estesp
estesp approved these changes Mar 16, 2022
View reviewed changes
Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@kzys
Copy link
Member Author

kzys commented Mar 16, 2022

Meantime Go 1.18 has been released: https://go.dev/dl/ Worth to either update/rebase this PR or open a new one.

Yey! I've just rebased the PR.

@kzys
Copy link
Member Author

kzys commented Mar 16, 2022

No GitHub Actions... I need to dig in.

https://github.com/containerd/containerd/pull/6605/checks

@zhsj
Copy link
Contributor

zhsj commented Mar 16, 2022

No GitHub Actions... I need to dig in.

https://github.com/containerd/containerd/pull/6605/checks

https://www.githubstatus.com/incidents/fpk08rxnqjz2

@estesp
Copy link
Member

estesp commented Mar 16, 2022

I just manually queued them, but we'll see if they show up here..eventually

@kzys
Copy link
Member Author

kzys commented Mar 16, 2022

@estesp Sorry. I actually made the PR smaller, wondering whether my fix broke GitHub Actions or not. Let's wait GitHub's recovery first.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 16, 2022

Build succeeded.

thaJeztah
thaJeztah approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda merged commit ed1a762 into containerd:main Mar 19, 2022
@tonistiigi
Copy link
Member

tonistiigi commented Mar 21, 2022

@AkihiroSuda Could we get this to 1.6 branch?

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 22, 2022

@AkihiroSuda Could we get this to 1.6 branch?

Any specific reason?

Usually we don't upgrade Go for backport branches (until reaching EOL)

@tonistiigi
Copy link
Member

tonistiigi commented Mar 22, 2022

The current release branches do not build with go1.18 (#6586) , meaning for example that we can't update buildkit to 1.18 . I don't care if the containerd CI starts using 1.18 or not but it should at least build with the latest stable Go.

@AkihiroSuda AkihiroSuda mentioned this pull request Mar 22, 2022
Closed
@thaJeztah
Copy link
Member

thaJeztah commented Mar 22, 2022

Yes the first commit can be backported

@thaJeztah
Copy link
Member

thaJeztah commented Mar 22, 2022

@tonistiigi I'm curious though; would GO111MODULE=off work as a (temporary) workaround? That should make it ignore go.mod, including the replace rule, and just look at the vendor directory. Looks like we're able to build the containerd binary in Moby, using go 1.18

@crazy-max crazy-max mentioned this pull request Mar 22, 2022
Merged
@dmcgowan
Copy link
Member

dmcgowan commented Mar 22, 2022

Agreed that we should have 1.6 building with Go 1.18 in CI. The 1.6 branch is young and will likely outlive Go 1.17.

@copybara-service copybara-service bot mentioned this pull request Apr 14, 2022
Closed
@copybara-service copybara-service bot mentioned this pull request Apr 14, 2022
Closed
copybara-service bot pushed a commit to google/gvisor that referenced this pull request Apr 14, 2022
@ayushr2 @gvisor-bot
Update BuildKite agents to use Go 1.17.
3557818 
As mentioned in containerd/containerd#6605, containerd 1.6 does not build
with go 1.18. Rollback buildkite agents to go 1.17 for now.

PiperOrigin-RevId: 441847492
@copybara-service copybara-service bot mentioned this pull request Oct 13, 2022
Closed
copybara-service bot pushed a commit to google/gvisor that referenced this pull request Oct 13, 2022
@ayushr2 @gvisor-bot
Update containerd tests to use versions 1.5.11 and 1.6.2.
e2d5663 
These versions are compatible with Go 1.18. There are a lot of OSS bugs
about this issue. Notable discussions:
- golang/go#51285
- containerd/containerd#6586

This was fixed upstream in the main branch in containerd/containerd#6605.
The fix was back-ported to v1.5 and v1.6 in containerd/containerd#6716
and containerd/containerd#6717 respectively.

These backports are available in the following versions:
```shell
/tmp/gopathJJGZf/containerd$ git tag --contains 765df66099eec88d0365eaa1e9a933877a058f0b
v1.6.2
v1.6.3
v1.6.4
v1.6.5
v1.6.6
v1.6.7
v1.6.8
/tmp/gopathJJGZf/containerd$ git tag --contains 86bec213720b54bf9ea2813e2c2a577271af58d3
v1.5.11
v1.5.12
v1.5.13
```

So use the smallest versions out of them.

PiperOrigin-RevId: 480965567
@thaJeztah thaJeztah added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed cherry-pick/1.5.x labels Jul 14, 2023
@thaJeztah thaJeztah mentioned this pull request Mar 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dims dims dims approved these changes

@estesp estesp estesp approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

No one assigned

Labels

cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.