search

brianmario / charlock_holmes

Character encoding detection, brought to you by ICU
MIT License
1.04k stars 141 forks source link

This simplifies the extconf and always builds with c++11 extensions #148

Closed tenderlove closed 5 years ago

tenderlove commented 5 years ago

We're having problems compiling this with Ruby 2.7. I think it always needs to have C++11 extensions enabled, so I changed the extconf to always enable them.

While I was messing with the extconf, I took the opportunity to clean it up a bit and use pkg-config for most of the dependencies.

mistydemeo commented 5 years ago

Does this change the minimum ICU version requirement? I know the move to making C++11 the default build happened a couple versions ago, but earlier versions weren't built as C++11 by default.

tenderlove commented 5 years ago

@mistydemeo I don't think so? AFAICT it just changes the compiler flags to allow the C++11 extension when compiling the Ruby extension (not ICU). I think always adding this flag is safe regardless of how ICU was built. I'm not 100% sure if what I just typed is true, but it sounds truthy, and we can't build this gem in our CI env without this flag.

mistydemeo commented 5 years ago

After some tests, I think this should be okay. I was concerned about a regression of #119, where always building C++11 broke the build on ICU 58 and lower, but that doesn't appear to be the case here. I've tested this branch against ICU 58.2, and both rake compile and rake test succeed.

tenderlove commented 5 years ago

I don't have write access to this repo so I can't merge. I'm willing to make a new release, etc if I can get write access. cc @brianmario

brianmario commented 5 years ago

@tenderlove added! Sorry about that! You're on most of my other repos and I'd assume that was the case here. And thanks for the help! :)

tenderlove commented 4 years ago

@brianmario would you mind adding me to the gem owners so I can ship a new version?

brianmario commented 4 years ago

Done!