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

Use atomic.Value to maintain Go 1.13 compatibility #582

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

hasheddan
Copy link
Contributor

Description

Updates to use atomic.Value in place of atomic.Bool such that older Go
versions can continue to be supported.

Signed-off-by: Daniel Mangum [email protected]

Similar fix to pion/transport@701ff64

@hasheddan hasheddan requested a review from Sean-Der September 5, 2023 11:54
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch coverage is 66.66% of modified lines.

Files Changed Coverage
internal/net/udp/packet_conn.go 66.66%

📢 Thoughts on this report? Let us know!.

Updates to use atomic.Value in place of atomic.Bool such that older Go
versions can continue to be supported.

Signed-off-by: Daniel Mangum <[email protected]>
Adds comment to indicate that conn ID should always be a string.

Signed-off-by: Daniel Mangum <[email protected]>
@daenney
Copy link
Member

daenney commented Sep 5, 2023

Honestly, I'd rather we don't and update the minimum version in go.mod instead.

Go 1.13 is from September 2019. It's now 4 years later. Go is very forwards compatible so the burden of upgrading a version once a while, especially once every 4 years, is basically 0. Go 1.19, which is what we'd need for atomic.Bool, is already a year old and with the release of 1.21 already no longer supported by the Go team. I think it's fine to move our baseline to it. That would also enable the use of generics if we want it at some point.

Copy link
Contributor Author

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Go 1.13 is from September 2019. It's now 4 years later. Go is very forwards compatible so the burden of upgrading a version once a while, especially once every 4 years, is basically 0. Go 1.19, which is what we'd need for atomic.Bool, is already a year old and with the release of 1.21 already no longer supported by the Go team. I think it's fine to move our baseline to it. That would also enable the use of generics if we want it at some point.

@daenney I tend to agree, but I believe there is a desire for other pion libraries to continue to support older Go versions, and other consumers are having to workaround this to maintain their older version support as well (e.g. plgd-dev/go-coap#481). Perhaps it makes more sense to maintain the compatibility for now, then start a discussion about what our minimum Go version requirement policy will be going forward?

@hasheddan hasheddan merged commit 2b584af into pion:master Sep 5, 2023
@daenney
Copy link
Member

daenney commented Sep 5, 2023

Go 1.13 is from September 2019. It's now 4 years later. Go is very forwards compatible so the burden of upgrading a version once a while, especially once every 4 years, is basically 0. Go 1.19, which is what we'd need for atomic.Bool, is already a year old and with the release of 1.21 already no longer supported by the Go team. I think it's fine to move our baseline to it. That would also enable the use of generics if we want it at some point.

@daenney I tend to agree, but I believe there is a desire for other pion libraries to continue to support older Go versions, and other consumers are having to workaround this to maintain their older version support as well (e.g. plgd-dev/go-coap#481). Perhaps it makes more sense to maintain the compatibility for now, then start a discussion about what our minimum Go version requirement policy will be going forward?

Yeah. I think that's a problem though since our CI also doesn't test those things anymore. But yup, lets have that discussion after this.

@at-wat
Copy link
Member

at-wat commented Sep 6, 2023

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.

3 participants