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

Move windows matcher logic so all platforms can use #22

Merged
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

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
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
}
}

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
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
platform_windows_compat.go Outdated Show resolved Hide resolved
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 <cpuguy83@gmail.com>
@kzys kzys merged commit e3566b8 into containerd:main Jan 11, 2025
7 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.

5 participants