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

Update go.mod to specify Go 1.19 as base #4330

Merged
merged 7 commits into from
Dec 26, 2023
Merged

Conversation

Jacalz
Copy link
Member

@Jacalz Jacalz commented Oct 18, 2023

Description:

Updating to Go 1.19 introduces support for a lot of new features like improved atomic types, possibility of using generics (though we ought to be careful there) and allows us to keep dependencies updated in the cases where they have dropped support for Go 1.17.

We can still compile with Go 1.17 at the moment for GopherJS support but that will be removed going forward and other Go 1.19 specific changes will also follow.

For #3242

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Any breaking changes have a deprecation path or have been discussed.

@Jacalz Jacalz mentioned this pull request Oct 18, 2023
3 tasks
@Jacalz
Copy link
Member Author

Jacalz commented Oct 18, 2023

I've had to remove GopherJS from our tests as they don't support anything over Go 1.17 in their stable releases. Their beta release has 1.18 support but there seems to be nothing happening with newer versions. I see no other option than removing support for it if we want to keep using modern Go versions...

@Bluebugs
Copy link
Contributor

That make it a bigger move. Currently wasm support is not a released one/ stable. Gopherjs is maintained by one person so far, so slow change. Wasm is currently being focused toward edge computing. Not to sure what's the best option here.

@Jacalz
Copy link
Member Author

Jacalz commented Oct 18, 2023

It is tough. We can't stay on 1.17 forever and there are a bunch of changes in Go 1.19 that we would benefit from. I don't think we have a choice honestly

@andydotxyz
Copy link
Member

Can we support the web concept with only WASM? It seems to be where people are putting their time and attention...

@Jacalz
Copy link
Member Author

Jacalz commented Oct 23, 2023

I would like to think so and presume that there is a reason that gopherjs isn't very popular. My suggestion would be to leave the gopherjs code in for now (just tweaking cmd/fyne to not list js as a target and maybe print some help text if someone tries to use it?) in hope that gopherjs gains improved support in the future.

@Jacalz
Copy link
Member Author

Jacalz commented Oct 23, 2023

But I suppose @Bluebugs has more opinions on the matter?

@metal3d
Copy link
Contributor

metal3d commented Oct 23, 2023

It seems that GopherJS is moving to 1.19 soon: gopherjs/gopherjs#1140

Generics are nearly to be supported.

I don't have any opinion... WASM works for me 😃 but GopherJS is, it seems, a bit more efficient.

@Jacalz
Copy link
Member Author

Jacalz commented Oct 23, 2023

Yeah, I read that as well. That was four months ago but no update since. While I do like to see that they are moving to 1.19, we are on Go 1.21 now and 1.22 is approaching soon. What's to say that we won't be held up by GooherJS next time we update our base as well?

PS: No offence to the people developing GooherJS. I don't doubt that it takes a lot of effort to maintain and they've done a fine job thus far. Just thinking about our project and our use case here :)

@Bluebugs
Copy link
Contributor

The future is likely wasm, but we are not there yet. With the latest go release it start to go into a good direction. Now, the likely future is that wasm will drop support for the "syscall/js" package which is source of most of the slowness. This drop will break our compatibility with go wasm at that point (go wasm has no stable API). I would expect this change to happen in not a too far future, maybe a year from now (this will require a lot of work in all our layer that support wasm to have it working again).

On the other hand, gopherjs is slowly keeping up with go and is currently faster. It's API is also stable.

So at some point, I expect our wasm port to be completely broken by a go release. And I hope that gopherjs by then would have catch up with the minimum version so that we can rely on it for that below broke the world release.

@Jacalz
Copy link
Member Author

Jacalz commented Oct 25, 2023

Hmm. That sounds no good at all. Sounds like we'll be stuck on Go 1.17 (or maybe Go 1.18 if GooherJS releases a new stable version) for some time then :/

@Jacalz
Copy link
Member Author

Jacalz commented Oct 29, 2023

I've thought some more about this and I came to wonder how many people are going to install an older version of Go just to compile and run their application? It seems to me like most people just use what is the latest and/or what their Linux/BSD distribution provides by default. There is a big difference between the case of supporting older Go versions and the case of not supporting new versions.

I think we need to discuss this on an upcoming contributor meeting and come to come sort of agreement.

@andydotxyz
Copy link
Member

There is a big difference between the case of supporting older Go versions and the case of not supporting new versions.

I would agree with this statement - but without this update are we really not supporting new Go versions?

@Jacalz
Copy link
Member Author

Jacalz commented Nov 27, 2023

There is a big difference between the case of supporting older Go versions and the case of not supporting new versions.

I would agree with this statement - but without this update are we really not supporting new Go versions?

I meant on the web support side of things. With GopherJS, anyone that wants to use it would have to always download an older Go version that neither receives bug fixes nor security fixes. I don't think a lot of people will want to install Go 1.17 just for serving with GopherJS support; especially not on Linux/BSD where people seem to mostly use what their package manager provides. We are trading a niece use case for the inability to use features in new Go versions.

@andydotxyz
Copy link
Member

I don't know what to do with this ticket. The GopherJS support is in to provide support for some browsers where the WASM does not work well. We could decide to take it out so there are fewer supported browsers, or we could choose to keep it in for a tougher developer experience.

As far as I can see there is nothing we are doing that actually requires go 1.19 so it seems like we don't have to make this trade-off at this time... Perhaps when we do have to there will be more info on supported versions or the non-WASM browsers will be far less popular?

@Jacalz
Copy link
Member Author

Jacalz commented Dec 14, 2023

The GopherJS support is in to provide support for some browsers where the WASM does not work well.

That’s the idea yes, but I doubt that many people are actually going to manually download Go 1.17 (which is far behind on security fixes among other things) just to be able to support older browser versions.

As far as I can see there is nothing we are doing that actually requires go 1.19 so it seems like we don't have to make this trade-off at this time...

That’s intentional. The actual changes to use newer APIs was intentionally not in this PR to keep it smaller and easier to review. There are a bunch of nice new APIs that I want to use in newer Go versions. The atomic variables in 1.19 would be wonderful.

@Bluebugs
Copy link
Contributor

The GopherJS support is in to provide support for some browsers where the WASM does not work well.

That’s the idea yes, but I doubt that many people are actually going to manually download Go 1.17 (which is far behind on security fixes among other things) just to be able to support older browser versions.

You don't need to. I'm the same way I don't have an environment for windows compilation, I can rely on fyne-cross for this.

@Jacalz
Copy link
Member Author

Jacalz commented Dec 14, 2023

You don't need to. I'm the same way I don't have an environment for windows compilation, I can rely on fyne-cross for this.

That solves the need to download Go 1.17 yes, but it doesn't change the fact that now are running binaries that are years behind on security updates (Go 1.17 is 2.5 years old and stopped reviving security updates 1.5 years ago). As the application developer, I'm basically responsible for putting my users at risk there in my opinion. While at the same time it is stopping adoption of newer APIs not just for Fyne but also for the project using Fyne with GooherJS.

@andydotxyz
Copy link
Member

andydotxyz commented Dec 15, 2023

That's not quite how things are - you don't have to compile with old Go just for GopherJS to work.
You can use newer go and just have 1.18/1.17 available and set the env GOPHERJS_GOROOT.

Yes gopherJS projects won't be able to use newer APIs, but given we were on 1.14 until recently is that really a big deal? Hopefully it gets resolved soon of course, but I don't see the need for hurry.

@andydotxyz
Copy link
Member

From the call today we discussed GopherJS and version requirements - along with our plans for web release etc.

Given that the web driver is not nearly ready, and that many of the dependencies are still in beta we had to come to the conclusion that release was not coming soon. And given this browser support will move on before we get there. So the proposal is that we explore the concept that "Web driver, when released, will target only WASM" and see where that takes us. Obviously it will mean less supported platforms, but by the time release it may be OK. And it takes the pressure of maintaining more code in the meantime...

@Jacalz
Copy link
Member Author

Jacalz commented Dec 15, 2023

Cool. Sounds like a very sensible conclusion from the discussion. I'm sorry that I couldn't attend. Will fix the conflicts and get this PR into shape

@coveralls
Copy link

coveralls commented Dec 23, 2023

Coverage Status

coverage: 64.332% (-0.3%) from 64.658%
when pulling 85cd02f on Jacalz:go1.19
into 62c45ee on fyne-io:develop.

@Jacalz
Copy link
Member Author

Jacalz commented Dec 23, 2023

I figured we can drop GopherJS in a future PR and also implement other Go 1.19 changes later on as well.

@Jacalz
Copy link
Member Author

Jacalz commented Dec 24, 2023

The web test requirement will have to be lifted for this to pass. I can revert back if you'd like

@andydotxyz
Copy link
Member

The web test requirement will have to be lifted for this to pass. I can revert back if you'd like

Not quite sure I follow - if you mean should it go back to testing on previous versions just for web then I guess so, as you say dealing with GopherJS would be a follow-up

@Jacalz
Copy link
Member Author

Jacalz commented Dec 24, 2023

I mean the requirement for the web test to pass. I updated it to test on latest Go as well so the action has a new name and no longer matches the workflow requirement

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Looking at the web - I cannot see it successfully building web in actions to replace the old web_tests action, am I missing something?

@@ -6,16 +6,19 @@ permissions:
jobs:
web_tests:
runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong - the matrix.os is not defined but above it is hard-coded to ubuntu

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right. Good catch

.github/workflows/web_tests.yml Outdated Show resolved Hide resolved
@Jacalz Jacalz changed the title Update minimum Go version to 1.19 Update go.mod to specify Go 1.19 as base Dec 25, 2023
@andydotxyz
Copy link
Member

Isn't that Windows 1.19 failure what we were seeing with 1.17 before?

@Jacalz
Copy link
Member Author

Jacalz commented Dec 25, 2023

Isn't that Windows 1.19 failure what we were seeing with 1.17 before?

Yes, of course. The fix is in 1.20 so we’re going to see it for a while more unless I patch it to run Windows on 1.20 instead.

@andydotxyz
Copy link
Member

Oh, I thought this was the test fix, sorry.
There was comments about getting the annoying test failures behind us - where was that?

@Jacalz
Copy link
Member Author

Jacalz commented Dec 25, 2023

I figured I'd merge this first and work on that later as it wasn't directly related. This one is just about raising the go.mod version

@andydotxyz
Copy link
Member

Ok, I guess I've been thinking about other things alongside this. What benefit does raising the version in go.mod if it's not for test fixes or using new features?

@Jacalz
Copy link
Member Author

Jacalz commented Dec 25, 2023

Sure, I reverted the update of fredbi/uri (that requires Go 1.19) so we could compile with Go 1.17 still and leave the web workflow intact (like you wanted) until the upcoming PR that removes GopherJS but that doesn't mean that we aren't going to use any new features from Go 1.19 going forward.

Like loosely implied in the README, I didn't want to bunch it all up in a single PR and instead opted to divide it up into multiple more easily-reviewed PRs (cleaning up code, switching to the new safer and faster atomic APIs, etc.).

As soon as this is merged, I plan to open new PRs and as such I'm kind of hesitant to get this to land soon.

@Jacalz
Copy link
Member Author

Jacalz commented Dec 25, 2023

The following PRs are basically going to try to implement the checkboxes in #3242 separated into multiple easily reviewed PRs of sensible size. Doing it all in this PR would be terrible.

@andydotxyz
Copy link
Member

Ok I guess I don't want to hold this back any longer, but please bear in mind:

  • This PR removes support for FreeBSD older than 5 years (< v12, dec 2018)
  • It does not yet deliver any new benefits enabled by the change (low hanging fruit might have been execabs removal?)
  • We have not finished the updates enabled by the recent increase to 1.17, or the intermediate 1.18.

I know this must be frustrating but given the number of platforms we juggle an upgrade like this should communicate the balance of loss with tangible benefits.

In previous discussions I had mistakenly thought this would fix our test failures.

@Jacalz
Copy link
Member Author

Jacalz commented Dec 26, 2023

This PR removes support for FreeBSD older than 5 years (< v12, dec 2018)

I believe that to not be an issue at all. The removed versions do not get security updates any more and newer versions do not remove hardware like Apple so it's fine.

@Jacalz Jacalz merged commit 3365e7f into fyne-io:develop Dec 26, 2023
11 of 12 checks passed
@Jacalz Jacalz deleted the go1.19 branch December 26, 2023 10:27
@Jacalz
Copy link
Member Author

Jacalz commented Dec 26, 2023

All your concerns will be resolved. You need not worry. Merry Christmas and happy new year ;)

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