-
Notifications
You must be signed in to change notification settings - Fork 1
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
v1.0.0.0: Migrate to libpcre2 #8
Conversation
Sorry, further testing has revealed this is not yet ready. Please bear with me and I'll update the pull request soon. |
7c0049f
to
0429621
Compare
I've had to do a lot more than I'd expected in the end! Anyway, I've also added a couple of smoke tests and they seem to be passing. It would be good to add tests with UTF-8 ByteStrings and usage of some options and corner cases where it might differ from the previous versions, but for now I'm just happy that it's working reasonably. |
test/.Main.hs.swp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ouch. Yeah, sloppy indeed. I'll remove it now.
I'm not a fan of this change. Let's rather have a new package, |
It was indeed something I considered and for sure it would be easy enough to do that. The main reasons I felt it would be better as a new version of this package are:
|
This is not true. You cannot have multiple versions of the same library. E.g. Stackage would be required to migrate at once. (Maybe there aren't many users of This is the very same reason why debian shipped old and new libpcre for quite some time. Perfectly, Abrupt change will be abrupt.
Probably every package which uses options will need to be updated, But to be fair, most packages use |
Thanks for the contribution! CI is failing... I updated CI on
If the problem is that additional packages are needed on the CI image, this could be possibly fixed by adding an |
When it comes to But maybe the users of |
Perhaps stackage cannot handle it, but ghc and cabal have no problems with multiple versions of the same lib. That said, I concede that it's better not to have this as it can lead to diamond dependencies.
It's not a direct dependency of HLS, but the intermediate dep could be updated indeed. |
Thanks. I'm not super familiar with Haskell CI, but I'll look into it. |
Fair enough. I'll do it that way then. |
regex-pcre2 is now on Hackage with a PR pending for Stackage. Thanks all for the comments/suggestions. |
Since the old libpcre is now obsolete for some years, migrating to libpcre2 is probably a good idea. This is a mostly-semantic API change that risks catching some people off guard, hence the major-major version number bump. I've kept the changes to a minimal set to maintain as much backward-compatibility as possible.
Fixes: #1