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

Move windows matcher logic so all platforms can use #22

Merged
merged 1 commit into from
Jan 11, 2025
Merged

Move windows matcher logic so all platforms can use #22

merged 1 commit into from
Jan 11, 2025

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jan 8, 2025

There shouldn't be any need to make the platform matcher stuff for Windows to only be available on Windows since none of it is dependent on the machine running it.

Related to moby/buildkit#5614

@cpuguy83 cpuguy83 requested a review from dmcgowan January 8, 2025 01:43
@cpuguy83 cpuguy83 mentioned this pull request Jan 8, 2025
Merged
tianon
tianon reviewed Jan 8, 2025
View reviewed changes
platforms.go Outdated Show resolved Hide resolved
@cpuguy83 cpuguy83 force-pushed the windows_everywhere branch 7 times, most recently from 39f21d0 to 4dce1e7 Compare January 8, 2025 20:59
@cpuguy83
Copy link
Member Author

cpuguy83 commented Jan 8, 2025

And now with CI passing! 🎉

@cpuguy83 cpuguy83 requested a review from tianon January 8, 2025 21:03
tianon
tianon reviewed Jan 8, 2025
View reviewed changes
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

A few questions that are probably comments on preexisting/moved code, but you've highlighted them again by moving so I've reviewed them 😅🙈😇

platform_windows_compat.go Show resolved Hide resolved
platform_windows_compat.go
}
}

func winRevision(v string) int {
Copy link
Member

Choose a reason for hiding this comment

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

Is this an implementation of a subset of the previous function? As is, this might return a value successfully where the above might return something empty, right? (That's kind of surprising behavior and might lead to subtle bugs; maybe it should just call the above function?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is different in that given a string like x.y.z.N it returns N where as the other function only looks at x.y.z

Copy link
Member

Choose a reason for hiding this comment

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

My worry here is that the other function returns the zero value but this one returns a number (foo.bar.baz.123), but I guess this one isn't really used in complete isolation because that last number is only meaningful if you know the other three are the same in the two values you're comparing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and is only used in the Less function.

platform_windows_compat.go Outdated Show resolved Hide resolved
@cpuguy83 cpuguy83 force-pushed the windows_everywhere branch from 4dce1e7 to 638b515 Compare January 9, 2025 17:02
dmcgowan
dmcgowan reviewed Jan 9, 2025
View reviewed changes
platforms.go Outdated Show resolved Hide resolved
@cpuguy83 cpuguy83 force-pushed the windows_everywhere branch from 638b515 to c2347f9 Compare January 9, 2025 22:22
kzys
kzys reviewed Jan 10, 2025
View reviewed changes
platform_windows_compat.go Outdated Show resolved Hide resolved
@cpuguy83
Move windows matcher logic so all platforms can use
7c58292 
There shouldn't be any need to make the platform matcher stuff for
Windows to only be available on Windows since none of it is dependent on
the machine running it.

Signed-off-by: Brian Goff <[email protected]>
@kzys kzys merged commit e3566b8 into containerd:main Jan 11, 2025
7 checks passed
@github-actions github-actions bot mentioned this pull request Jan 19, 2025
Open
Mengkzhaoyun pushed a commit to open-beagle/containerd that referenced this pull request Feb 7, 2025
Merge tag 'v2.0.2' into release-v2.0
884b2c6 
containerd 2.0.2

Welcome to the v2.0.2 release of containerd!

The second patch release for containerd 2.0 includes a number of bug fixes and improvements.

* Remove confusing warning in cri runtime config migration ([#11256](containerd/containerd#11256))
* Fix runtime platform loading in cri image plugin init ([#11248](containerd/containerd#11248))

* Update runc binary to v1.2.4 ([#11239](containerd/containerd#11239))

Please try out the release binaries and report any issues at
https://github.com/containerd/containerd/issues.

* Jin Dong
* Derek McGowan
* Akihiro Suda
* Kazuyoshi Kato
* Henry Wang
* Krisztian Litkey
* Phil Estes
* Samuel Karp
* Sebastiaan van Stijn
* Akhil Mohan
* Brian Goff
* Chongyi Zheng
* Maksym Pavlenko
* Mike Brown
* Pierre Gimalac
* Wei Fu
<details><summary>23 commits</summary>
<p>

* Prepare release notes for v2.0.2 ([#11245](containerd/containerd#11245))
  * [`cdaf4dfb4`](containerd/containerd@cdaf4df) Prepare release notes for v2.0.2
* Update platforms to latest rc ([#11259](containerd/containerd#11259))
  * [`eb125e1dd`](containerd/containerd@eb125e1) Update platforms to latest rc
* Remove confusing warning in cri runtime config migration ([#11256](containerd/containerd#11256))
  * [`468079c5c`](containerd/containerd@468079c) Remove confusing warning in cri runtime config migration
* Fix runtime platform loading in cri image plugin init ([#11248](containerd/containerd#11248))
  * [`a2d9d4fd5`](containerd/containerd@a2d9d4f) Fix runtime platform loading in cri image plugin init
* make sure console master tty is closed on task exit ([#11246](containerd/containerd#11246))
  * [`184ffad01`](containerd/containerd@184ffad) Add integ test to check tty leak
  * [`17181ed33`](containerd/containerd@17181ed) fix master tty leak due to leaking init container object
* Bump up otelttrpc to 0.1.0 ([#11242](containerd/containerd#11242))
  * [`8666e7422`](containerd/containerd@8666e74) Bump up otelttrpc to 0.1.0
* ctr: `ctr images import --all-platforms`: fix unpack ([#11236](containerd/containerd#11236))
  * [`c4270430d`](containerd/containerd@c427043) ctr: `ctr images import --all-platforms`: fix unpack
* Update runc binary to v1.2.4 ([#11239](containerd/containerd#11239))
  * [`7373ddd70`](containerd/containerd@7373ddd) update runc binary to v1.2.4
* downgrade go-difflib and go-spew to tagged releases ([#11222](containerd/containerd#11222))
  * [`f34147772`](containerd/containerd@f341477) downgrade go-difflib and go-spew to tagged releases
* Add a build tag to disable std `plugin` import ([#11213](containerd/containerd#11213))
  * [`dca769485`](containerd/containerd@dca7694) chore: add a build tag to disable containerd plugin import
* Update golangci to 1.60.3 ([#11187](containerd/containerd#11187))
  * [`5942b3fcb`](containerd/containerd@5942b3f) Update golangci to 1.60.3
</p>
</details>
<details><summary>6 commits</summary>
<p>

* Add dependabot and upgrade golang and dependency versions ([containerd/otelttrpc#3](containerd/otelttrpc#3))
  * [`2d46141`](containerd/otelttrpc@2d46141) upgrade golang, deps, CI versions
  * [`64922e7`](containerd/otelttrpc@64922e7) Add dependabot CI
* Fix concurrent map panic on metadata ([containerd/otelttrpc#2](containerd/otelttrpc#2))
  * [`2ba3be1`](containerd/otelttrpc@2ba3be1) Fix concurrent map panic on inject metadata
  * [`f50a922`](containerd/otelttrpc@f50a922) UT for concurrent inject/extract metadata
</p>
</details>
<details><summary>6 commits</summary>
<p>

* Move windows matcher logic so all platforms can use ([containerd/platforms#22](containerd/platforms#22))
  * [`7c58292`](containerd/platforms@7c58292) Move windows matcher logic so all platforms can use
* replace testify with stdlib in tests ([containerd/platforms#21](containerd/platforms#21))
  * [`86a86b7`](containerd/platforms@86a86b7) replace testify with stdlib in tests
* Replace arm64 minor variant logic with lookup table ([containerd/platforms#18](containerd/platforms#18))
  * [`364665a`](containerd/platforms@364665a) Replace arm64 minor variant logic with lookup table
</p>
</details>
<details><summary>5 commits</summary>
<p>

* Add MD.Clone function ([containerd/ttrpc#177](containerd/ttrpc#177))
  * [`430f734`](containerd/ttrpc@430f734) Add MD.Clone
* server: fix a Serve() vs. (immediate) Shutdown() race ([containerd/ttrpc#175](containerd/ttrpc#175))
  * [`c4d96d5`](containerd/ttrpc@c4d96d5) server: fix Serve() vs. immediate Shutdown() race.
  * [`ed6c3ba`](containerd/ttrpc@ed6c3ba) server_test: add Serve()/Shutdown() race test.
</p>
</details>

* **github.com/containerd/otelttrpc**  ea5083fda723 -> v0.1.0
* **github.com/containerd/platforms**  v1.0.0-rc.0 -> v1.0.0-rc.1
* **github.com/containerd/ttrpc**      v1.2.6 -> v1.2.7
* **github.com/davecgh/go-spew**       d8f796af33cc -> v1.1.1
* **github.com/pmezard/go-difflib**    5d4384ee4fb2 -> v1.0.0
* **github.com/stretchr/testify**      v1.9.0 -> v1.10.0

Previous release can be found at [v2.0.1](https://github.com/containerd/containerd/releases/tag/v2.0.1)
* `containerd-<VERSION>-<OS>-<ARCH>.tar.gz`:         ✅Recommended. Dynamically linked with glibc 2.31 (Ubuntu 20.04).
* `containerd-static-<VERSION>-<OS>-<ARCH>.tar.gz`:  Statically linked. Expected to be used on non-glibc Linux distributions. Not position-independent.

In addition to containerd, typically you will have to install [runc](https://github.com/opencontainers/runc/releases)
and [CNI plugins](https://github.com/containernetworking/plugins/releases) from their official sites too.

See also the [Getting Started](https://github.com/containerd/containerd/blob/main/docs/getting-started.md) documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tianon tianon tianon left review comments

@kzys kzys kzys approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@profnandaa profnandaa profnandaa approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@cpuguy83 @kzys @tianon @dmcgowan @profnandaa