Skip to content

Remove support for GHC < 9.0#119

Merged
jinwoo merged 3 commits intogoogle:masterfrom
blackgnezdo:forget-ghc8
Feb 9, 2026
Merged

Remove support for GHC < 9.0#119
jinwoo merged 3 commits intogoogle:masterfrom
blackgnezdo:forget-ghc8

Conversation

@blackgnezdo
Copy link
Contributor

@blackgnezdo blackgnezdo commented Dec 5, 2024

Remove conditional source code for ghc < 9.0

@blackgnezdo
Copy link
Contributor Author

@jinwoo WDYT? Time to take out this garbage? People who need the old ghc will have the old releases and we won't have to worry about #109 ever again.

@jinwoo
Copy link
Member

jinwoo commented Dec 5, 2024

Sorry we're still stuck with GHC 8.10 and I'd like to still keep it. But I don't work with Haskell anymore. @agrue What do you think?

@blackgnezdo
Copy link
Contributor Author

Sorry we're still stuck with GHC 8.10 and I'd like to still keep it. But I don't work with Haskell anymore. @agrue What do you think?

I don't think it matters to you. You can keep using the old version that's already imported in //third_party.

Alternatively, if you want to be sure that we don't break it going forward and the new versions still work on 8.10, somebody should add this version to the build matrix.

@agrue
Copy link

agrue commented Dec 7, 2024

It's a bit scary, because this is a dependency of proto-lens. So if proto-lens eventually depends on a new version of ghc-source-gen that only works with GHC 9+, we won't be able to upgrade to that version. And unfortunately the "you can stay on the old version" logic doesn't apply there, because the proto ecosystem keeps evolving (e.g. we just recently had to add support for "proto editions" because proto files we depend on started using those features).

So if we have to do a similar thing in the future, we'll be stuck unable to import the new proto-lens version with support for the new features. Unless we try to back-port those features onto the old internal version but... ugh.

I don't know how to weigh this against the needs of external users though. If some other maintainer wants to merge this I don't feel like I can stop you.

@blackgnezdo
Copy link
Contributor Author

It's a bit scary, because this is a dependency of proto-lens. So if proto-lens eventually depends on a new version of ghc-source-gen that only works with GHC 9+, we won't be able to upgrade to that version. And unfortunately the "you can stay on the old version" logic doesn't apply there, because the proto ecosystem keeps evolving (e.g. we just recently had to add support for "proto editions" because proto files we depend on started using those features).

So if we have to do a similar thing in the future, we'll be stuck unable to import the new proto-lens version with support for the new features. Unless we try to back-port those features onto the old internal version but... ugh.

I have two points to offer here:

  • ghc-source-gen last received API changing updates over 3 years ago (gasp, I was still a googler!)
  • proto-lens can be the place to enforce the dependency on a 8.10 compatible version of this library (it even has 8.10 in its build matrix)

I don't know how to weigh this against the needs of external users though. If some other maintainer wants to merge this I don't feel like I can stop you.

I don't feel strongly enough to advocate for this really. This PR can simply stay open until something changes...

@agrue
Copy link

agrue commented Dec 7, 2024

That makes sense to me. Okay, well if you ever do feel strongly about it, I can't really argue against it.

Update base from >=4.7 to >=4.15 and ghc from >=8.4 to >=9.0 to match
the Hackage revision and CI build matrix, which don't support GHC <9.0.

Fixes google#128
@blackgnezdo
Copy link
Contributor Author

blackgnezdo commented Feb 9, 2026

@jinwoo I rebased this PR. It is good to come clean about the fact that we don't have <9.0 support any longer in the last release anyway.

@jinwoo
Copy link
Member

jinwoo commented Feb 9, 2026

@blackgnezdo Sorry we're still stuck with 8.10. Can we keep the support for a bit more? Do you have any immediate needs to clean this up? This will be fine as long as we don't import the new changes but I'm a bit worried about the cases where we need to upgrade due to some security vulnerabilities or something.

@blackgnezdo
Copy link
Contributor Author

@blackgnezdo Sorry we're still stuck with 8.10. Can we keep the support for a bit more? Do you have any immediate needs to clean this up? This will be fine as long as we don't import the new changes but I'm a bit worried about the cases where we need to upgrade due to some security vulnerabilities or something.

That's the thing: the current version in master is broken on 8.10. We can either revive 8.10 support and make all the changes going forward progressively harder, or acknowledge the reality of not supporting anything <9.0 any more as in #129 and then reap the benefits of cleaner code from this PR.

@jinwoo
Copy link
Member

jinwoo commented Feb 9, 2026

8.10 is broken in master? I thought that the CI was set up for >= 9.0 but the code still worked with 8.10. Is that my misunderstanding?

Anyway I do agree that it's good to clean up this old code. I was hesitant just because of our situation here. I'm running the CI now and will merge if it passes.

@blackgnezdo
Copy link
Contributor Author

8.10 is broken in master? I thought that the CI was set up for >= 9.0 but the code still worked with 8.10. Is that my misunderstanding?

#128 is the report which alerted me of this.

Anyway I do agree that it's good to clean up this old code. I was hesitant just because of our situation here. I'm running the CI now and will merge if it passes.

Thanks

@jinwoo
Copy link
Member

jinwoo commented Feb 9, 2026

8.10 is broken in master? I thought that the CI was set up for >= 9.0 but the code still worked with 8.10. Is that my misunderstanding?

#128 is the report which alerted me of this.

Oh, I see. Then this change seems needed. Thanks for making the change.

CI passed. Merging now.

@jinwoo jinwoo merged commit 985e3ad into google:master Feb 9, 2026
10 checks passed
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