Fix building under older go releases #200
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #200 +/- ##
==========================================
+ Coverage 81.60% 81.70% +0.10%
==========================================
Files 32 32
Lines 2750 2750
==========================================
+ Hits 2244 2247 +3
+ Misses 396 394 -2
+ Partials 110 109 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Hi @jech - can you explain the 'raison d'etre' behind this? Go 1.15 (and also 1.16) are EOL. So without knowing any context I would say the right approach is to build Galene with Go 1.17/1.18, not to fuxge Go packages so they build on EOL versions of the compiler. |
|
The main raison d'être, as you say, is the second patch, which fixes compilation with gccgo (now that Pion can be built with it, golang/go#52512). While I was at it, I fixed compilation with older Go versions. Transport's go.mod file declares compatibility with 1.12, which means that anyone trying to build transport with 1.15 or earlier is going to get a confusing error message. This can be fixed either by bumping the version in go.mod, or by applying this patch; either is fine with me, but I'd prefer you simply applied the patch, since I feel that Go's one-year obsolescence policy is way too aggressive for many Linux distributions. |
|
@jech - I'm still not sure about this. (Sorry, I am not trying to be difficult, rather, I am trying to be thoughtful.) I agree a one-year obsolescence is aggressive. But I understand the logic behind a short obsolescence window, which includes making life easier for package authors since this policy removes the need to backtest on N > 1 old compilers. Because of this, you would not expect package authors in general to test things on old compilers. Since presumably Galene and others depend on other packages beyond Pion, it isn't clear to me that the 'right' approach here is for Pion to offer a more permissive policy on EOL compiler support. This could open a can of worms. For example: right now for our CI testing we just test on Go 1.17 and Go 1.18. If we are making a change to support Go 1.15 or 1.16 then we would also need to add those compilers to our CI testing. What then happens if Pion as it stands fails testing on those compilers? Someone is going to have to trawl through other peoples' code and figure out why. And even if not, we are then doubling the compiler support burden for future contributors to Pion who now have to support and/or test on 1.15/1.16 even though they are developing on (presumably) 1.18. I just feel like this is adding contributor friction and/or opportunities for things to break. As always my mind is open to being convinced otherwise but I just think adding support for 1.15 would be net negative for the project. If we are now otherwise supporting gccgo, with the exception of the usage of GAS, then I am fine with your 2nd patch. |
|
Oh well. Please feel free to ignore the first patch, just fix compilation with gccgo. |
|
@jech - can you split this into two PRs? The title of this is 'fix building under older go releases' which doesn't seem to be what about the 2nd patch is about. I can then approve the PR to address building with gccgo, and we can leave the other PR (about building under older go releases) open for further thought/discussion. |
|
So you're asking me to do extra work for purely ideological reasons? |
|
@jech I think splitting up the commits will help people debugging in the future. I doubt something will happen (but if it does) it will help people a lot if they can revert individual commits to understand. People also may lose confidence in Pion if they see multiple topics in one commit. I don't know how much it matters, but I am less likely to contribute to projects where I feel the authors aren't passionate about the code. My 2 cents, but I see where you both are coming from! |
|
@Sean-Der It's two distinct commits. The first one adds backwards-compatible If the consensus is that Pion does not compile with versions of Go earlier than 1.16, then somebody needs to change the version declared in go.mod so that the compiler can generate a useful error message. Having the version declared as 1.12 while the code only supports 1.16, which is the current situation, makes no sense to me at all. |
|
@jech - there actually wasn't anything ideological about my request (although there's nothing wrong with ideological requests). It was purely practical. I don't know how to effectively merge 'half' a PR in Github. Given that our CI is testing only 1.17 and 1.18, for all intents and purposes we only support 1.17 and 1.18, and so I definitely think we should be clear that we only support those compiler versions. I am neutral about whether we enforce that or not (e.g. by changing the go.mod), but I think that should be clear to current and future Pion contributors. |
Please don't do that. If you decide to do a fix different than the one that I propose, please do your own testing and commit it under your own name, since I do not wish to have my name associated with a partial fix that is not the one that I've implemented and tested myself. If you believe that I deserve credit, add "Thanks to Juliusz Chroboczek" or even " |
|
@jech - first and foremost I wanted to make clear that I (and I think everyone here who knows you and/or has seen your work) have the utmost respect for your PRs and your views. My job as a maintainer here is largely to challenge and question things and I want to make sure that, when I do have questions and don't immediately spring to merge commits or accept PRs, there is no disrespect intended. I feel that something may have got lost in translation here so I wanted to make sure I said that explicitly. This PR, in particular, raises a very good question about what Go compiler versions Pion should support. As you rightly point out, right now the situation is a bit of a 'hodge podge' and we definitely need to resolve this. It isn't clear to me what the 'right' answer is and, to be honest, given that any decision we make is likely to have side effects on third-party projects that use Pion as a dependency, I feel quite strongly this is something that needs community input and I don't feel confident to make unilateral decisions. Let's leave this PR open for the time being, and meanwhile I will open an issue on the main |
|
I think what's declared in |
|
I found this MR after trying to update Galène and having the build fail with an obscure error ( 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. |
This fixes compilation with go1.15 and earlier by adding old-style build tags.
This fixes building under gccgo by disabling all assembler code.
|
I think we should be pragmatic here and support the Go versions which are shipped with stable versions of most common distros. Its, also not a very intrusive change. Removing the old build tags is an easy change which we need to do Pion-wide anyways once the time has come. Hence, I support this PR to be merged :) |
|
Hi all, I've merged the branch for the following reasons:
I also started a poll in the Pion Slack channel some time ago about whether we should raise the minimum supported Go version. Most members supported the idea. However, concerns have been raised about Debian stable which currently just ships with 1.15. Since the next stable version of Debian is landing this year, I propose to keep support for 1.15 for the time beeing and switch to 1.18 once its released. |
I appreciate that. However, it's been almost a year, so I expect that most potential users have moved on to other SFUs, that they found easier to compile. What's more, Galene no longer builds with older Go releases, since I've been unable to test due to this issue. OTOH, it now builds with gccgo-12, so there's at least that. |
CC @adriancable