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

Drop support for GHC < 8 #89

Merged
merged 6 commits into from
Aug 3, 2022
Merged

Drop support for GHC < 8 #89

merged 6 commits into from
Aug 3, 2022

Conversation

TeofilC
Copy link
Contributor

@TeofilC TeofilC commented Jul 9, 2022

This MR removes parsers and workarounds specific to GHC < 8.
I've also removed some test cases for older eventlogs.

Resolves #88

Also drop related workarounds and tests that depend on this code.

Strangely one of the GHC-7 tests failed as well even though the comments
suggest this code is only needed for GHC-6. This isn't an issue cause we
are dropping support for GHC-7 too.
@Mikolaj
Copy link
Member

Mikolaj commented Jul 9, 2022

@bgamari, @mpickering, @AndreasPK: since you are the only producers of ghc-events data and you keep a copy of the file .eventlog format in GHC --- do you see any obstacle to dropping compatibility with 5 years old GHCs? How could this affect your copy of the code?

@maoe
Copy link
Member

maoe commented Jul 10, 2022

I don't use GHC < 8.0 any more so I'm personally fine with this change.

I feel like dropping support for those old versions would help simplify the library and unblock some of the open issues.

I'm interested in this remark. What issue does this change unblock specifically?

@TeofilC
Copy link
Contributor Author

TeofilC commented Jul 10, 2022

I think I had this comment in mind #14 (comment)

@AndreasPK
Copy link
Contributor

@bgamari, @mpickering, @AndreasPK: since you are the only producers of ghc-events data and you keep a copy of the file .eventlog format in GHC --- do you see any obstacle to dropping compatibility with 5 years old GHCs? How could this affect your copy of the code?

I don't think there should be issues for ghc itself in that regard.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 11, 2022

I don't think there should be issues for ghc itself in that regard.

Thank you. In that case, even code cleanup could be reason enough. After all, maintenance takes work and hairy code makes it harder.

@maoe
Copy link
Member

maoe commented Jul 13, 2022

I think I had this comment in mind #14 (comment)

I see. If that issue could be fixed by simply dropping the old GHC support as suggested, that would be great.

I'm +1 to this change.

I'm going to ping @simonmar as he said

ghc-events was specifically designed to be forwards and backwards compatible as far as possible.

in #27 in the past. Do you mind if we drop support for GHC < 8 now?

@maoe
Copy link
Member

maoe commented Jul 13, 2022

@TeofilC Have you checked if this PR fixes #14?

@TeofilC
Copy link
Contributor Author

TeofilC commented Jul 13, 2022

It doesn't fix #14 unfortunately. I tried re-enabling the test suites and they still fail. I think the remaining issue is due to events being reordered because of how EventBlock is handled.

@maoe
Copy link
Member

maoe commented Aug 3, 2022

It doesn't fix #14 unfortunately.

That's unfortunate.

That said, it'd still be nice to get rid of some old code along with the relevant test data for simplicity. Also there have been no objections since this was submitted almost a month ago. I think we can go ahead.

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.

Drop support for GHC < 8
4 participants