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

change go minimum version in README #4416

Merged
merged 1 commit into from
Oct 4, 2024

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
@@ -27,16 +27,10 @@ A third party security audit was performed by Cure53, you can see the full repor

## 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.

README.md Outdated Show resolved Hide resolved
@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!

@@ -27,16 +27,10 @@ A third party security audit was performed by Cure53, you can see the full repor

## 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.

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
42 checks 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.

6 participants