search

cloudflare / lua-aho-corasick

BSD 3-Clause "New" or "Revised" License
152 stars 51 forks source link

CFLAGS etc. #12

Closed vcunat closed 5 years ago

vcunat commented 6 years ago

When I see things like -msse4.1 in ./Makefile, I wonder whether you intend the master branch of this repo for general-purpose usage, e.g. ending up in distribution packages. Should I make similar changes in a fork or shall we generalize it directly here, perhaps in a way that you might keep injecting such flags somehow?

Currently the binaries will end up in knot-resolver packages with these flags (even on ARM :) I suspect this will be a real problem for x86 systems without SSE 4.1, and most users surely won't notice a significant difference in speed if we drop these flags. (I understand you might notice the difference on CloudFlare servers.)

vcunat commented 6 years ago

There are also other weird things I see, e.g. passing CFLAGS to c++ compiler, etc. so I'd probably look at these as well.

vcunat commented 5 years ago

OK, I created some implementation instead of just talking: https://github.com/cloudflare/lua-aho-corasick/pull/13

pspacek commented 5 years ago

@vavrusa Could you please poke someone to get fixes merged? Thanks!

vcunat commented 5 years ago

Or just get some "authoritative" opinion; it's not a big deal for us to maintain a fork, but it would seem better if we can agree on one common version.

jdesgats commented 5 years ago

Hey, sorry for the late answer. It turns out we already fixed this issue internally, I pushed the corresponding patches in #14

vcunat commented 5 years ago

Well, this is a misunderstanding between us – my concern was that by default the code should work on any CPU of the given architecture. Old example: x86_64 does not guarantee sse4.1; new example: aarch64 does not guarantee the CRC instructions. I actually suspect that our device which already runs this code doesn't have CRC, but I've been primarily concerned about compatibility from the point of view of generic packaging for unix-like systems.

jdesgats commented 5 years ago

You are right, we should not hard-code CPU-specific compile flags (being amd64 or ARM). I guess that a proper configure script or cmake file could detect them automatically, but we don't have that so far so your PR is the best way forward.

Sorry again, I ogt busy on many other things and I needed to check where this module is used in our internal repos in order to not have any regressions.

jdesgats commented 5 years ago

Solved by merging #13.