Provide common cmake project interface. - #2153
Conversation
The old include paths are preserved by adding an additional "<BUILD_INTERFACE>/pcapplusplus" directive. Fixed `CotopLayer.cpp` which relied on `../headers/CotpLayer.h` instead of using the include directories.
Remove alias definition in Examples.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2153 +/- ##
==========================================
- Coverage 82.79% 82.79% -0.01%
==========================================
Files 331 331
Lines 60047 60040 -7
Branches 12388 12657 +269
==========================================
- Hits 49714 49708 -6
+ Misses 9452 9450 -2
- Partials 881 882 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I don't like moving all headers under 1. Generate a "shim" directory of forwarding headers (most common alternative) // build/generated_include/pcapplusplus/ArpLayer.h
#include "ArpLayer.h"Then add that generated directory to the cmaketarget_include_directories(PacketPP PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/generated_include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/header>
$<INSTALL_INTERFACE:include>
$<INSTALL_INTERFACE:include/pcapplusplus>
)This is what a lot of projects do (e.g. via |
|
I somewhat dislike shims, tbh. I have seen them used for backwards compatibility for when headers are moved and no other option is available, but I would prefer to avoid them otherwise, since they generate build system churn. From #1586 and #1789, I thought the long term goal was to eventually migrate to Why do you dislike the headers being moved under |
What do you mean by that? Can you give examples?
If we put the common interface aside, this folder structure seems weird and redundant: I need to think about it more, but maybe it'd make more sense to have a structure like this? |
Mostly that they generate files that aren't strictly speaking needed. On another note, the shims would break if we want to have a switch to enforce strict conformance to the package style The entire shim architecture stems requires keeping the old style
Hmm, option 1 does appear a bit redundant. I am not super happy with it either, but it is a somewhat common convention as it is simple to setup and maintain out of the box as it works identical for both build and install interfaces. No extra steps needed. The install step is also a simple copy. The issue I have with option 2 is that it removes all project module boundaries (module per top level folder). I think it might have been fine if they all compiled to one target, but we have |
4 participants
Background
There are two common ways CMake packages are consumed: as an installed package or as a subdirectory. Currently the way
PcapPlusPlusis consumed between the two methods is inconsistent in the following ways:#includesignatures can differ.#include <pcapplusplus/ArpLayer.h>is only available on<INSTALL_INTERFACE>.PcapPlusPlus::Pcap++vsPcap++Standardization of include signatures
PR #1789, added a new include signature
#include <pcapplusplus/ArpLayer.h>. This was a partial implementation of #1586, with the intention of eventually transitioning fully to the<pcapplusplus/xxx>syntax. However the change was not ported to the<BUILD_INTERFACE>.This inconsistency is addressed by moving all public headers into a subdirectory
pcapplusplus(e.g.header/ArpLayer.hbecomesheader/pcapplusplus/ArpLayer.h). The change allows<BUILD_INTERFACE>to also utilize the include syntax#include <pcapplusplus/ArpLayer.h>.Breaking changes and backwards compatibility
The relocation of the header files in a subfolder has the potential to break the following include signatures:
Direct include paths:
#include "./<pcppRoot>/headers/ArpLayer.h". These paths do not rely on the include directory mechanism and explicitly define a path from their source to the include file. Since usage of these paths is rare across project boundaries, no compatibility layer was provided.Old style include paths:
#include "ArpLayer.h". To keep backwards compatibility, the<BUILD_INTERFACE>defines two include directories for the project headers:<pcppRoot>/headersand<pcppRoot>/headers/pcapplusplus. This allows both#include <ArpLayer.h>and#include <pcapplusplus/ArpLayer.h>to be resolved correctly.The old signatures should eventually be deprecated, but this is outside the scope of this PR.
Standardization of CMake targets
CMake targets that are being imported from an installed package generally come in a
Package::Targetnaming convention. This differs from normal targets which generally lack thePackage::prefix. This makes all targets included from a subdirectory inconsistent with their imported counterparts. To solve that problem, CMake provides anALIAStarget type, which is a pseudo target that acts as a read-only symbolic link to an actual target.The PR adds the following ALIAS targets to standardize the CMake target API:
PcapPlusPlus::Common++(alias forCommon++)PcapPlusPlus::Packet++(alias forPacket++)PcapPlusPlus::Pcap++(alias forPcap++)The following targets were not provided with an alias target, even though they are exported during install:
light_pcapng- The target is generally exported as a byproduct of an internal dependency. It should not be linked directly.Examples/folder.Minor fixes
CotpLayer.hto utilize include paths instead of relying on relative path../headers/CotpLayer.h.Additional notes
#include <ArpLayer.h>signatures for the moment.