pion / webrtc

Pure Go implementation of the WebRTC API
https://pion.ly
MIT License
13.84k stars 1.66k forks source link

Which Go compiler versions should Pion support? #2292

Closed adriancable closed 7 months ago

adriancable commented 2 years ago

This issue was 'inspired' by https://github.com/pion/transport/pull/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 commented 2 years ago

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

Here's an example of a perfectly friendly exchange fixing breakage on an older version: https://github.com/pion/ice/pull/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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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:

Thoughts?

silbe commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 1 year ago

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 commented 7 months ago

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

@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 commented 7 months ago

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

Sean-Der commented 7 months ago

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

Thank you so much for doing this @edaniels !

edaniels commented 7 months ago

Woo! Tag teamed!