devel/hyperscan: Install shared library, Modernize suricata 3.1 added support for Hyperscan, but requires linking against its shared library. The current version of this port has a SHARED option which is not enabled by default. This prevents users of the suricata (and other software from having hyperscan support, which is desirable by default. In its default configuration (not in OPTIONS_DEFAULT), it also prevents Ports users from having hyperscan support, without hunting for the reason why, creating a poor user experience. It is a reasonable expectation that software that can provide a shared library for consumption & linking by other software, should install that library by default, particularly in the context of FreeBSD Ports and most other packaging systems, where linking against shared (vs static) libraries is a de-facto, if not a required standard or expectation. - Remove the SHARED option accordingly. - Ask (CMAKE) for both the static and shared libraries to be built. - Add USE_LDCONFIG. - Update pkg-plist accordingly. While I'm here: - Remove core2 from C{XX}FLAGS when NATIVE is off. It is overly prescriptive for a global default, and can be overriden by user-supplied CFLAGS, defeating it's purpose. If the aim was to provide optimized builds, perhaps these flags would be better suited to sub-packages tailored to particular CPU's or Archs (core2, etc). Instead of the above, patch CmakeLists.txt to add an option (WITH_NATIVE) to build with -m{arch,tune}=native when enabled, and not fiddle with *FLAGS when it isn't enabled instead. This could be upstreamed. It shouldn't be the default. Remove NATIVE_C{XX}FLAGS options helpers accordingly, as it's already done by CmakeLists.txt. In addition: Add -mssse3 to C{XX}FLAGS instead and add a pre-install: conditional check for the CPU feature using MACHINE_CPU, setting BROKEN if the feature is not detected. (See Also: multimedia/ffmpeg) Remove ONLY_FOR_ARCHS_amd64 to allow building on other architectures. - Use USES=pathfix instead of post-stage: foo for s/lib/libdata fix. - Remove custom DEBUG check, USES=cmake handles build targets/stripping. - Add patch to CMakeLists.txt adding WITH_EXAMPLES as an option. - Use OPTIONS helpers for the NATIVE option. - Update NATIVE_DESC and ONLY_FOR_ARCHS_REASON verbiage. - Repatch patch files (makepatch compliant). - Add LICENSE_FILE. - Sort and group USE{S} entries. - Sort pkg-plist. - Add test target. PR: 211002 Reviewed_by: vsevolod (maintainer), mat Approved_by: vsevolod (maintainer) DiffRev: DXXXX MFH: 2016Q3
Details
- Reviewers
vsevolod
- portlint: OK (false positive WARN)
- WARN: Makefile: NATIVE is listed in OPTIONS_DEFINE, but no PORT_OPTIONS:MNATIVE appears
- portlint missing CMAKE_BOOL in list of option helper vars to check
- porttest: OK (poudriere)
- 93amd64 (gcc48)
- 103amd64 (clang)
- 103i386 (clang)
- 11amd64 (clang)
- unittest: OK
- 2577 tests from 30 test cases ran. (910856 ms total)
- [ PASSED ] 2577 tests.
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 4674 Build 4728: arc lint + arc unit
Event Timeline
devel/hyperscan/Makefile | ||
---|---|---|
32 | I'll fix this |
devel/hyperscan/Makefile | ||
---|---|---|
46–49 | I don't understand why this is in a target, it should be by itself. |
devel/hyperscan/Makefile | ||
---|---|---|
46–49 | The aim was to prevent installation, the narrowest limitation i could think of, because runtime wont work on a machine without SSSE3 instructions, but one can built it without issue. What's a better way? |
devel/hyperscan/Makefile | ||
---|---|---|
46–49 | What's the point of allowing building of something you can't use ? |
devel/hyperscan/Makefile | ||
---|---|---|
46–49 | Also, this does not do what you think it does. This will create a pre-install target that does one thing, run a variable assignment in its shell, but it does not define BROKEN in the Makefile. |
devel/hyperscan/Makefile | ||
---|---|---|
46–49 | Cross building or building packages on the cluster (which we obviously dont want to impact) for a wider class of host CPU's (that do have the SSSE3 instructions) What's the best way to achieve this? |
devel/hyperscan/Makefile | ||
---|---|---|
46–49 | So, you want to handle the convoluted case where the package building machine does not have SSE3 but the machine using the package has, while not allowing the building machine without SSE3 to install it. Then echo the message in the pre-install target, and end it with ${FALSE}, also, enclose the pre-install: line in the .if so that you don't end up with an empty target. But let me say this, it is very very ugly. Now, SSE3 arrived like 12 years ago, I'd say not doing that but just adding a pkg-message saying "you need SSE3 for this to work" would be better. |
From what we can tell, especially the "illegal instruction trouble" (see below), we need a suricata-hs package that adds hyperscan without tainting the original package and make the new one amd64 only.
Or leave it as a non-default option for the brave ports builders out there.
I don't see how the hyperscan package can be improved without them stopping to require at least SSE3.
Also see: https://github.com/opnsense/core/issues/1244
Cheers,
Franco
@franco_opnsense.org The rest of these changes have maintainer approval and are pending commit (by me). I'll consider the idea of a separate port/package against the WIP port I have locally (which enables packages on supported archs by default)
I'm not doubting these changes, I just saw that enabling hyperscan for suricata is problematic for non SSE3 archs and there's no way around this :/
Speaking of which, I'll drop a patch for 3.1.3 into the bug tracker in a second. :)