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

Which Go compiler versions should Pion support? #2292

Closed
adriancable opened this issue Aug 5, 2022 · 16 comments
Closed

Which Go compiler versions should Pion support? #2292

adriancable opened this issue Aug 5, 2022 · 16 comments

Comments

@adriancable
Copy link
Contributor

This issue was 'inspired' by pion/transport#200 created by @jech which has the effect of broadening the range of Go compiler versions that Pion can be built with. I felt the question of Go compiler support is best answered with substantial community input as it impacts past and future contributors to Pion, as well as third-party software that uses Pion as a dependency.

The core question is: which versions of the Go toolchain should Pion support? Right now there is no clear answer (i.e. nothing is stated explicitly in the docs), and the Pion codebase itself offers a number of contradictory answers:

  1. Pion's CI system tests builds only against 1.17 and 1.18. This is consistent with Go's own support for the current major version, plus the last version, of the toolchain. This means that, in practice, committed changes are only guaranteed to work on 1.17 and 1.18.
  2. go.mod in the main repo targets go 1.13. Some of the other packages e.g. pion/transport target go 1.12 in the go.mod.
  3. Recent changes e.g. inclusion of optimised assembly for crypto functions currently use syntax only available in 1.16 onwards.

The point was raised that the Go toolchain advances faster than some Linux distros are updated, so it is possible/likely that people are developing primary with Go toolchain releases that are now unsupported/EOL.

Some possible options:

  1. We synchronize Pion's support for the Go toolchain with Go's own support schedule, i.e. current version + last major version only. This is effectively what's done by Pion's current CI system, and so this 'change' is simply to state that 1.17 and 1.18 are the only supported Go toolchain releases. We can choose to enforce this in go.mod or not do so, and state that Pion may compile under earlier versions of Go but we do not test or support such usage.
  2. We make a decision to broaden support to e.g. current + last + last-but-one + last-but-two. If we do this we should change Pion's CI process to also build and test under 1.16 and 1.15. This might reveal breakages either due to Go features in use in the codebase that were not supported in 1.15/1.16, or due to compiler bugs whose fixes were not marked for backport. This might be fine, or might open a 'can of worms' - it is hard to tell.

What are people's views on this?

@jech
Copy link
Member

jech commented Aug 6, 2022

Until recently, Pion has been using a system that apparently works for everyone:

  • people would submit improvements to Pion, without going out of their way to ensure it works with older copilers;
  • occasionally, somebody would notice that Pion no longer works on a version of Go that they still care about, and submit a fix;
  • occasionally, we'd remove obsolete code when we're reasonably sure that nobody cares any longer.

Here's an example of a perfectly friendly exchange fixing breakage on an older version: pion/ice#361.

This system has been working well, and has allowed people who care about older compilers to use a recent version of Pion, which is good for the project since it gets timely testing. I don't understand why you have suddenly decided to replace it with something different, that is obviously causing pointless friction and that would cause me to stick to an older version of Pion.

@adriancable
Copy link
Contributor Author

adriancable commented Aug 6, 2022

@jech - I'm not sure I understand.

I don't understand why you have suddenly decided to replace it with something different

This is exactly what I am not doing. Per my post in pion/transport I do not think that any single individual (including myself) should be making unilateral decisions on versioning policy, because these decisions potentially have wide community impact. That's why I am asking for broader views (beyond me and you), so that whatever the consensus is, we can codify it so there is no ambiguity, and both future contributors and Pion users know what compilers we support. I am not 'vetoing' your PR on broadening compiler support for a particular package (I don't have the ability to 'veto' anything), rather, I am using this as an opportunity to establish what our version support policy is, and write it down. If as a community we agree that our policy should be best-effort support for older compilers, for example, then I will merge the PR in a flash.

My point is not that anything done to date is wrong, rather, because we don't have anything written down on what this project's policy is on compiler support, there is the ongoing risk that two people disagree on what the policy actually is. What is happening now is a good example of what I want to avoid in the future. So, I am asking for broader views on what our policy should be, so we can write it down, and then there is no ambiguity. I am not 'replacing it with something different', and in fact I do not have any views on what the policy should be. I would hope this is clear from my above post when I presented a number of different options, without expressing any preference myself. My sole wish is that, as a community, we agree 'a view' and then write it down, so there is no ambiguity.

@Sean-Der, @davidzhao and others - I would appreciate your views.

@mengelbart
Copy link
Contributor

@adriancable I understand the wish to have an agreement as a community on what compilers Pion supports and I think that what @jech described above worked well in the past. One problem I see with this approach is that it is somewhat unclear when contributors are allowed to use new language features, which most of the time is not a big issue, but might be one soon, now that both of the last supported compiler versions support generics. I don't know how much they are actually used, so maybe that is nothing to worry about, but in the worst case, I think we may lose contributors by requiring them to implement patches without using new language features.

I think we should certainly avoid situations such as in pion/transport where the code is incompatible with what is declared in go.mod.

@adriancable
Copy link
Contributor Author

@mengelbart - thanks for this - I don't have any issue with @jech's proposal for a system at all, but I think we (A) need to reconcile it with what happens on the contributor/CI side, which is currently that we only build and test for 1.17 and 1.18 (this may be 1.18 and 1.19 by now), and (B) write it down. My issue is not with any specific policy but with 'folklore' policies in general that are not written down. This just causes confusion, unless you have been following how things work at Pion for a long time. We shouldn't make 'learning folklore' a requirement to contribute to Pion.

I absolutely agree that code should not use language features that are not supported by what is declared in go.mod. (In the interest of transparency, I am guilty of this in at least one place in pion/transport.) My uncertainty is that there are 2 ways to fix this - remove the language features, or bump the version in go.mod. The former is what @jech would like and appears in line with the 'folklore' policy in effect. The latter would enable us to sync up the module versions with what we actually support, i.e. what the Pion CI system builds and tests against (1.17/1.18).

If we are happy with the policy that @jech described then please let's write it down! We also need to make it consistent with what the CI system does. Some possible wording we could add to the README that might achieve both things:

  • Pion aims to match Go's own policies on toolchain version support, that is, the current and previous major versions of the compiler. At the time of writing this means that Pion supports Go versions 1.18 and 1.17 (probably 1.19 and 1.18 by now). Pion's CI system builds and tests against these Go versions.
  • We recognize that older versions of Go are still in use by developers. We cannot guarantee compatibility with EOL versions of the Go toolchain, because we do not build or test against these compiler versions. However, if you do have an issue building or running with an EOL compiler version and have a fix for this, we are open to accepting PRs (provided that they do not break compatibility with supported versions of the toolchain).

Thoughts?

@silbe
Copy link

silbe commented Sep 8, 2022

Repeating parts of what I just wrote on pion/transport#200:

I'm running Galène (which depends on this project) as a production server on Debian stable which ships Go 1.15. Since this is a production environment I'd very much prefer to stick with the Go version it was shipped with (which has full security support) rather than using the one from backports (which may or may not get security updates).

If there's significant effort required to continue supporting a Go version for the lifetime of Debian stable (usually about 2 years) I can understand you'd prefer not to. I'll update to Go 1.18 from backports in that case. But at the very least the build should fail in a way that makes it obvious the reason for the failure is that I need a more recent Go version.

@davidzhao
Copy link
Member

Jumping into this late. Been thinking about this one for a bit.

I support having an official "version policy" around Go version support. My reasons are:

  • set clarity and expectations for contributors around compatibility expectations
  • stating what we won't support will reduce the perceived difficulty of contributing
  • it'd be hugely beneficial to adopt a version where newer features like generics could be used. we don't want Pion to be a project to be forever stuck on 1.15.

It's important to make the distinction that this is a developer-impacting decision that does not impact the end-users. As with all Go binaries, there are no dependencies on having a particular toolchain installed for end-users.

What are reasons for folks that develop with Pion to keep using Go 1.15/1.16?

@adriancable
Copy link
Contributor Author

@davidzhao - one of the reasons I have heard is that Linux distros don't tend to update their Go toolchain as often as they could/should.

I'm not sure how much of a practical block this really is, though. The Go toolchain has been designed to be simple to install and update (copy and paste one command line into the terminal). @jech - do you know from Galene users why some people don't want to use more modern / supported versions of Go?

@jech
Copy link
Member

jech commented Sep 11, 2022

I'm sorry, but I don't have either the time or the inclination to have this discussion.

In the meantime, however, compilation under gccgo is still broken. So please either apply pion/transport#200 or provide a different but tested fix yourself

@adriancable
Copy link
Contributor Author

adriancable commented Sep 11, 2022

@jech - that's a shame. This discussion started, I believe, as a consequence of Galene users being frustrated by the inability to compile under older versions. If you 'drop out' at this point and we lose an understanding of this data point, it makes it harder to make good decisions (= more likely we will continue in this holding pattern for longer than we need to).

The gccgo compilation issue isn't (as I understand it) related to the Go version support discussion. Because I do not have or use gccgo, I can't provide a fix for this that I have tested, but I will gladly merge a PR submitted by anyone who does have the ability to test. (not pion/transport#200 as it stands, since that PR makes other changes not related to an uncontentious fix for gccgo)

@jech
Copy link
Member

jech commented Sep 11, 2022

This discussion started, I believe, as a consequence of Galene users...

I'm pretty sure that's not the case. You started this discussion single-handed, thus creating a problem where there was none, and causing extra work for everyone.

@adriancable
Copy link
Contributor Author

adriancable commented Sep 11, 2022

@jech - it's actually the opposite. By actually defining a toolchain support policy we will save people a lot of work in the future (e.g. writing code using generics, and then being told after the fact it isn't allowed and having to rewrite - or the opposite, e.g. writing code without generics when they could have saved development time by using generics). I acknowledge that this discussion requires effort from its participants in order to make a good decision for users and maintainers, but Pion like any open source project relies on that.

If anyone else feels that I am causing extra work for everyone, they haven't yet said so, so I will disregard that particular comment for the time being as it is probably inaccurate, but am always open to hear views from others.

@edaniels
Copy link
Member

edaniels commented Sep 6, 2023

Based on where the project (all repos) is currently at, I feel like the next major version would be an appropriate point to have a toolchain adoption and obsolescence schedule. I say this because things are pretty mature at this point and it would be a shame to not be able to take improvements from new language versions. It also encourages new developers with go to contribute, especially if they're used to knowing "new" things recently introduced. Pion is a prime example of good and modern go engineering and I'd love to have that be easier to keep that way!

@Sean-Der
Copy link
Member

This was discussed at the Pion quarterly meeting in Slack. Consensus was that 1.19 would be an acceptable upgrade

  • 1.19 is what Debian stable is at. This seems to be the platform where users are stuck on older versions
  • 1.19 does give us some features that people are excited for

@edaniels If you are interested want to make https://github.com/pion/.goassets/blob/master/scripts/lint-go-mod-version.sh fail the build. I can help make the PRs across all the repos if you like!

Happy to take the issue myself, but this seemed like something you were passionate about

@edaniels
Copy link
Member

Yup I'll give it a stab. May need help but I'll lyk

Sean-Der added a commit that referenced this issue Apr 2, 2024
Sean-Der added a commit to pion/turn that referenced this issue Apr 2, 2024
Sean-Der added a commit to pion/transport that referenced this issue Apr 2, 2024
Sean-Der added a commit to pion/turn that referenced this issue Apr 2, 2024
Sean-Der pushed a commit to pion/ice that referenced this issue Apr 3, 2024
Sean-Der pushed a commit to pion/interceptor that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/sctp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/sdp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/srtp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/stun that referenced this issue Apr 3, 2024
Sean-Der pushed a commit to pion/dtls that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/rtcp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/sdp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/srtp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/srtp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/stun that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/template that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/transport that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/srtp that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/stun that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/turn that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/turn that referenced this issue Apr 3, 2024
Sean-Der added a commit that referenced this issue Apr 3, 2024
Sean-Der added a commit to pion/turn that referenced this issue Apr 3, 2024
Sean-Der added a commit that referenced this issue Apr 3, 2024
Sean-Der added a commit that referenced this issue Apr 3, 2024
Sean-Der added a commit that referenced this issue Apr 3, 2024
@Sean-Der
Copy link
Member

Sean-Der commented Apr 3, 2024

Done! go.mod version is 1.19 across all the repos.

Thank you so much for doing this @edaniels !

@Sean-Der Sean-Der closed this as completed Apr 3, 2024
@edaniels
Copy link
Member

edaniels commented Apr 3, 2024

Woo! Tag teamed!

ourwarmhouse added a commit to ourwarmhouse/Smart-Contract---Go that referenced this issue May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants