Skip to content

Conversation

@amghazanfari
Copy link
Contributor

In the readme there are two statements

  1. the minimum version of go for runc is v1.21
  2. if you use go v1.22 make sure it's newer than 1.22.4 due to a bug

when i use go v1.21 in Dockerfile for testing I got error

+ exec make localunittest TESTFLAGS=
rm -f libcontainer/dmz/binary/runc-dmz
go generate -tags "seccomp urfave_cli_no_docs" ./libcontainer/dmz
go: go.mod requires go >= 1.22 (running go 1.21.13; GOTOOLCHAIN=local)
make: *** [Makefile:116: runc-dmz] Error 1
make: *** [Makefile:161: unittest] Error 2

but with go v1.22 and v1.23 it was ok. so i change minimum version to 1.22.4 and delete the warning

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

My bad; PR #4360 should have done that.

Comment on lines -34 to -39
#### Go

NOTE: if building with Go 1.22.x, make sure to use 1.22.4 or a later version
(see [issue #4233](https://github.com/opencontainers/runc/issues/4233) for
more details).

Copy link
Contributor

Choose a reason for hiding this comment

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

To other reviewers: this is no longer needed since PR #4407.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i change 1.22.4 to 1.22 in my changes?

Copy link
Member

Choose a reason for hiding this comment

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

IMHO just removing this is fine. I think @kolyshkin was just giving context to other reviewers on why removing that note seems fine.

@kolyshkin
Copy link
Contributor

kolyshkin commented Sep 26, 2024

Apparently actuated removed bats from the default software set, this is why actuated CI fails // Cc @alexellis

Nah, there's something wrong with cache restoration logic in actions/bats-setup

@lifubang
Copy link
Member

Nah, there's something wrong with cache restoration logic in actions/bats-setup

Resolved in #4412

@kolyshkin
Copy link
Contributor

Resolved in #4412

Merged, rebased this one.

@alexellis
Copy link

Hi @kolyshkin nothing has been removed, and bats was never part of the image AFAIK.

Make sure any cache keys you use are unique between actuated/hosted runners as permissions, users and paths may vary - so a cache tar taken from a hosted runner may not restore exactly as expected onto an self-hosted runner and visa-versa.

Let me know if I can help

Calyptia and Fluent both use bats extensively with actuated and I'm not aware of any issues from them. They may not have the cache option enabled for bats?

@kolyshkin
Copy link
Contributor

Hi @kolyshkin nothing has been removed, and bats was never part of the image AFAIK.

Make sure any cache keys you use are unique between actuated/hosted runners as permissions, users and paths may vary - so a cache tar taken from a hosted runner may not restore exactly as expected onto an self-hosted runner and visa-versa.

Let me know if I can help

Calyptia and Fluent both use bats extensively with actuated and I'm not aware of any issues from them. They may not have the cache option enabled for bats?

My bad, I mixed things up and mentioned you too early. The issue was in us starting to use https://github.com/bats-core/bats-action, which lacked the arch in cache key, so caches for different archtitectures got mixed. This was promptly reported and fixed by @akhilerm.

Again, thank you so much for allowing runc to use Actuated; it's great and invaluable addition to GitHub Actions.

README.md Outdated
## Building

`runc` only supports Linux. It must be built with Go version 1.21 or higher.
`runc` only supports Linux. It must be built with Go version 1.22.4 or higher.
Copy link
Member

Choose a reason for hiding this comment

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

Now that go enforces the go version in go.mod, don't we want to just delete the last sentence here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine as it is.

Comment on lines -34 to -39
#### Go

NOTE: if building with Go 1.22.x, make sure to use 1.22.4 or a later version
(see [issue #4233](https://github.com/opencontainers/runc/issues/4233) for
more details).

Copy link
Member

Choose a reason for hiding this comment

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

IMHO just removing this is fine. I think @kolyshkin was just giving context to other reviewers on why removing that note seems fine.

@AkihiroSuda
Copy link
Member

Please squash the commits

Signed-off-by: Amir M. Ghazanfari <[email protected]>

Update go version

Co-authored-by: Akihiro Suda <[email protected]>
Signed-off-by: Amir M. Ghazanfari <[email protected]>
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

## Building

`runc` only supports Linux. It must be built with Go version 1.21 or higher.
`runc` only supports Linux. See the header of [`go.mod`](./go mod) for the required Go version.
Copy link
Member

Choose a reason for hiding this comment

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

This was changed. If we are going to have some text, the text before seem better. But I'm fine with this.

@kolyshkin what do you think? You weighted on this before.

@AkihiroSuda AkihiroSuda requested a review from kolyshkin October 3, 2024 16:59
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

lgtm

@kolyshkin kolyshkin merged commit 84529ba into opencontainers:main Oct 4, 2024
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.

6 participants