Force pcapplusplus prefix in include

[clementperon]

At the moment building a simple example like

CMakeLists.txt

      project(TestPcapPlusPlus)
      set(CMAKE_CXX_STANDARD 11)
      find_package(PcapPlusPlus CONFIG REQUIRED)
      add_executable(test test.cpp)
      target_link_libraries(test PUBLIC PcapPlusPlus::Pcap++)
      set_target_properties(test PROPERTIES NO_SYSTEM_FROM_IMPORTED ON)

test.cpp

      #include <cstdlib>
      #include <pcapplusplus/PcapLiveDeviceList.h>
      int main() {
        const std::vector<pcpp::PcapLiveDevice*>& devList =
          pcpp::PcapLiveDeviceList::getInstance().getPcapLiveDevicesList();
        if (devList.size() > 0) {
          if (devList[0]->getName() == "")
            return 1;
          return 0;
        }
        return 0;
      }

Trig an error on MacOS when PcapPlusplus is installed with brew

VERBOSE=1 cmake --build build
[ 50%] Building CXX object CMakeFiles/test.dir/test.cpp.o
/Library/Developer/CommandLineTools/usr/bin/c++  -I/opt/homebrew/include/pcapplusplus -std=gnu++11 -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk -MD -MT CMakeFiles/test.dir/test.cpp.o -MF CMakeFiles/test.dir/test.cpp.o.d -o CMakeFiles/test.dir/test.cpp.o -c /Users/clement/work/test-pcappp/test.cpp
/Users/clement/work/test-pcappp/test.cpp:2:16: fatal error: 'pcapplusplus/PcapLiveDeviceList.h' file not found
    2 |       #include <pcapplusplus/PcapLiveDeviceList.h>
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[2]: *** [CMakeFiles/test.dir/test.cpp.o] Error 1
make[1]: *** [CMakeFiles/test.dir/all] Error 2
make: *** [all] Error 2

This is because the include folder is " -I/opt/homebrew/include/pcapplusplus" and file is located in /opt/homebrew/include/pcapplusplus/PcapLiveDeviceList.h

Remove the pcapplusplus folder

[Dimi1010]

~~Any reason this merges in the master branch?~~

Nvm, the change of base branch was not updated for me.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 83.08%. Comparing base (0dea70c) to head (2278cfc). :warning: Report is 143 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1586      +/-   ##
==========================================
- Coverage   83.08%   83.08%   -0.01%     
==========================================
  Files         283      283              
  Lines       48954    48954              
  Branches    10383    10335      -48     
==========================================
- Hits        40675    40673       -2     
+ Misses       7175     7150      -25     
- Partials     1104     1131      +27     

Flag	Coverage Δ
alpine320	75.06% <100.00%> (ø)
fedora42	75.15% <100.00%> (-0.04%)	:arrow_down:
macos-13	80.60% <50.00%> (ø)
macos-14	80.60% <50.00%> (+<0.01%)	:arrow_up:
macos-15	80.57% <50.00%> (ø)
mingw32	70.69% <100.00%> (-0.04%)	:arrow_down:
mingw64	70.66% <100.00%> (-0.04%)	:arrow_down:
npcap	85.07% <ø> (-0.02%)	:arrow_down:
rhel94	74.94% <100.00%> (+<0.01%)	:arrow_up:
ubuntu2004	58.54% <100.00%> (-0.03%)	:arrow_down:
ubuntu2004-zstd	58.68% <100.00%> (+0.02%)	:arrow_up:
ubuntu2204	74.85% <100.00%> (-0.04%)	:arrow_down:
ubuntu2204-icpx	61.50% <50.00%> (ø)
ubuntu2404	75.09% <100.00%> (-0.04%)	:arrow_down:
ubuntu2404-arm64	75.10% <100.00%> (ø)
unittest	83.08% <100.00%> (-0.01%)	:arrow_down:
windows-2019	85.09% <ø> (-0.01%)	:arrow_down:
windows-2022	85.12% <ø> (-0.01%)	:arrow_down:
winpcap	85.25% <ø> (ø)
xdp	50.56% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[seladb]

@clementperon I remember we discussed it, but I forgot why we need this change. Where does homebrew expect the header files to be, and where are they now?

[clementperon]

@clementperon I remember we discussed it, but I forgot why we need this change. Where does homebrew expect the header files to be, and where are they now?

@seladb

Homebrew will install the files in "/opt/homebrew/Cellar/pcapplusplus/24.09/include/pcapplusplus/xxx.h"

The actual CMake will provide the following include directory

• "/opt/homebrew/Cellar/pcapplusplus/24.09/include/pcapplusplus"

This MR make the proper change to give this directory

• "/opt/homebrew/Cellar/pcapplusplus/24.09/include"

We could also keep a fallback compatibility by including both folder for the moment:

• "/opt/homebrew/Cellar/pcapplusplus/24.09/include/pcapplusplus"
• "/opt/homebrew/Cellar/pcapplusplus/24.09/include"

But it's proper to use "pcapplusplus/xxxx.h" everywhere in our code no ?

[seladb]

@clementperon Here are my thoughts:

• This is a major breaking change because any project that uses PcapPlusPlus would need to adjust their #include. A solution could be placing the header files in both /opt/homebrew/Cellar/pcapplusplus/24.09/include/pcapplusplus and /opt/homebrew/Cellar/pcapplusplus/24.09/include, but it's not very nice...
• I think that PcapPlusPlus code doesn't need to change and should still #include "xxx.h". Only the tests and examples should use the new #include pcapplusplus/xxx.h
• As we previously discussed I don't think the new structure of having pcappplusplus/include in Common++, Packet++ and Pcap++ is very nice

I'm wondering if it's worth the change, or what are the other options we have 🤔

[clementperon]

@clementperon Here are my thoughts:

• This is a major breaking change because any project that uses PcapPlusPlus would need to adjust their #include. A solution could be placing the header files in both /opt/homebrew/Cellar/pcapplusplus/24.09/include/pcapplusplus and /opt/homebrew/Cellar/pcapplusplus/24.09/include, but it's not very nice...

To me the final user should always use

#include <pcapplusplus/XXXX.h>

Which is at the moment broken on MacOS and can also break on Linux depends where you install PcapPlusPlus. On linux it mostly works because the header are installed in /usr/local/include and /usr/local/include is a default path.

If we want to keep the compatibilty with

#include "XXXX.h"

Then we can include both folder in the CMake and it will work without breaking nobody.

• I think that PcapPlusPlus code doesn't need to change and should still #include "xxx.h". Only the tests and examples should use the new #include pcapplusplus/xxx.h

I agree, but tests and examples are libraries that have the possibility to be built in the pcapplusplus project. I could fix that, if we don't link with library like we do at the moment. see: https://github.com/seladb/PcapPlusPlus/blob/master/Examples/CMakeLists.txt#L16C15-L16C27

• As we previously discussed I don't think the new structure of having pcappplusplus/include in Common++, Packet++ and Pcap++ is very nice

I'm wondering if it's worth the change, or what are the other options we have 🤔

[seladb]

@clementperon Here are my thoughts:

* This is a major breaking change because any project that uses PcapPlusPlus would need to adjust their `#include`. A solution could be placing the header files in both `/opt/homebrew/Cellar/pcapplusplus/24.09/include/pcapplusplus` and `/opt/homebrew/Cellar/pcapplusplus/24.09/include`, but it's not very nice...

* I think that PcapPlusPlus code doesn't need to change and should still `#include "xxx.h"`. Only the tests and examples should use the new `#include pcapplusplus/xxx.h`

* As we previously discussed I don't think the new structure of having `pcappplusplus/include` in `Common++`, `Packet++` and `Pcap++` is very nice

I'm wondering if it's worth the change, or what are the other options we have 🤔

Let's add more people to the discussion to hear their thoughts: @tigercosmos @egecetin @Dimi1010 , what do you think?

[Dimi1010]

Somewhat of a late response but here are my thoughts.

I think the change is worth it, as it more explicitly shows which library owns which headers for the users, and reduces the chances of name conflicts.

I think that PcapPlusPlus code doesn't need to change and should still #include "xxx.h". Only the tests and examples should use the new #include pcapplusplus/xxx.h

I personally prefer using full include paths everywhere as I find it less trouble with the compiler, but I'm fine either way.

As we previously discussed I don't think the new structure of having pcappplusplus/include in Common++, Packet++ and Pcap++ is very nice

I have found it that it saves me trouble to have the pattern <LibProject>/include/<libname>/xxx.h for include directories in the source tree. Not the prettiest to look at but its simple to install afterwards.

While we are on the topic of having a folder for the public include headers. I think it might be a good idea to also have subfolders for the subprojects, so the include tree is something like this:

- pcapplusplus
-- common -> headers below
-- packet -> headers below
-- pcap -> headers below

The rationale is to remove the possibility of naming conflicts between projects and to more explicitly show which header is part of which subproject now that #1763 will allow building the project without the Pcap++ submodule.

[tigercosmos]

Continue above, I agree with @Dimi1010. I expect something like

#include <pcapplusplus/common/xxxx.h>
#include <pcapplusplus/packet/xxxx.h>
#include <pcapplusplus/pcap/xxxx.h>

[seladb]

I'm not sure our users care whether it's Common++, Packet++ or Pcap++, for them it's just PcapPlusPlus headers...

I think the bigger problem here is if/how we maintain backward compatibility? Or do you think we should say it's a breaking change and library users need to update all their includes?

[tigercosmos]

I'm not sure our users care whether it's Common++, Packet++ or Pcap++, for them it's just PcapPlusPlus headers...

Either way is fine. I saw both in other libraries.

I think the bigger problem here is if/how we maintain backward compatibility? Or do you think we should say it's a breaking change and library users need to update all their includes?

The old design doesn't follow common sense in the C++ world. So I guess a breaking change should be fine.

[Dimi1010]

I'm not sure our users care whether it's Common++, Packet++ or Pcap++, for them it's just PcapPlusPlus headers...

They might, if they want to compile only parsing support, and suddenly they have to dig in the project files to see which headers are part of the parsing module and which are part of the capture module.

[seladb]

I'm not sure our users care whether it's Common++, Packet++ or Pcap++, for them it's just PcapPlusPlus headers...

They might, if they want to compile only parsing support, and suddenly they have to dig in the project files to see which headers are part of the parsing module and which are part of the capture module.

@Dimi1010 if they only compile parsing support they will only have the header files they need, so that shouldn't be a problem

The old design doesn't follow common sense in the C++ world. So I guess a breaking change should be fine.

@tigercosmos my concern is that if someone uses PcapPlusPlus heavily, it'll be a pretty serious breaking change for them which will make it harder for the to bump the version

[Dimi1010]

@tigercosmos my concern is that if someone uses PcapPlusPlus heavily, it'll be a pretty serious breaking change for them which will make it harder for the to bump the version

We can have transition headers that are basically an xxx.h header that just has #include pcapplusplus/.../xxx.h. And a warning preferrably.

[seladb]

@tigercosmos my concern is that if someone uses PcapPlusPlus heavily, it'll be a pretty serious breaking change for them which will make it harder for the to bump the version

We can have transition headers that are basically an xxx.h header that just has #include pcapplusplus/.../xxx.h. And a warning preferrably.

Interesting idea! How is it going to work?

[Dimi1010]

@tigercosmos my concern is that if someone uses PcapPlusPlus heavily, it'll be a pretty serious breaking change for them which will make it harder for the to bump the version

We can have transition headers that are basically an xxx.h header that just has #include pcapplusplus/.../xxx.h. And a warning preferrably.

Interesting idea! How is it going to work?

@seladb Essentially you have the following structure (packet subfolder is just to show it can work through subfolders):

header/pcapplusplus/packat/AAXXX.hpp

#pragma once
#include <string>

namespace pcpp
{
	class AAXXX
	{
	public:
		static std::string getAAXXXName()
		{
			return "AAXXX";
		}
	};
}

header/AAXXX.hpp

#pragma once
#include "pcapplusplus/packet/AAXXX.hpp"

That way if someone has #include "AAXXX.hpp" it will still work, but will allow them to change the includes to #include "pcapplusplus/.../AAXXX.hpp" at their leasure.

Ideally we would want to have #warning message in the transition header too, but that is C++23 standard :/

Also if we do this, the idea is that any new headers won't have a transition header added, to encourage ppl to move to the new headers.

Pros and cons overview is thus:

• Pros - the change is no longer a breaking change
• Cons - Every existing public header will need to leave a transition header when its moved that acts as a symbolic link to include the new location.

[seladb]

@Dimi1010 I think your solution could work! Maybe we can even generate these "symbolic link" files automatically during build time so they're not part of the project. I think it's pretty easy to write a python script to generate them, however I'm not sure how to run this script from CMake 🤔

[Dimi1010]

@seladb It should be able to generate them at build time. Shouldn't be that hard to do it with pure CMake even.

Although that way, it would probably be easier to generate a transition header for all public headers, not just the current ones. Or we need to keep a snapshot of which headers to create a transition header for.

[seladb]

@seladb It should be able to generate them at build time. Shouldn't be that hard to do it with pure CMake even.

Although that way, it would probably be easier to generate a transition header for all public headers, not just the current ones. Or we need to keep a snapshot of which headers to create a transition header for.

Yes, I think generating them automatically for all public headers is the better option

[Dimi1010]

@seladb Added automatic generation of the transition headers. I think that will be good enough?

Only the install interface considers these files as in the "include" directory. The build interface does not use them at all, so we don't use them in new code.

PS: Android CI works because of the backwards compatibility change, not because its actually fixed.

[Dimi1010]

@seladb Scrapped the transition headers idea due to having the critical issue of polluting the global include on install.

I think just keeping the $<INSTALL_INTERFACE:include/pcapplusplus> in addition to the new $<INSTALL_INTERFACE:include> might also work?

[Dimi1010]

@seladb Got a bit of a question for desired implementation of backwards compat.

It should be possible to have both the old and new install #include format if we essentially keep both for target_include_directories. Currently I think I can make it essentially be controlled by a flag, via a CMake generator expression. If the flag is set, the old style interfaces will also be added.

The flag will need to be added to the config file, if we want to default it to ON (as I assume we would want that to keep backwards compat). I also think it would be a good idea to add a deprecation warning to the cmake config file if the flag is set to on, as the regular one in the CMakeLists.txt will only run if building the project from source.

The question I have is, should the old way be available for users to turn it off, or have it always on? What do you think?

This is the include directories expression with the optional flag:

$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/header>
$<INSTALL_INTERFACE:include>
$<$<BOOL:PCAPPP_USE_OLD_INCLUDE>:$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/header/pcapplusplus>$<INSTALL_INTERFACE:include/pcapplusplus>>

[seladb]

@Dimi1010 just to make sure I understand, mostly because I'm not a CMake expert: do we want to have a CMake flag to control whether to have both the new and old include format? And the default will be ON which mean include both?

If we have this flag and it's turned off I think it can leave only the new format. Let me know if this answers your question

[Dimi1010]

The question is more:

"Do we want the flag to be there to allow people to disable the old includes, or do we want them to be always on until we remove them?"

[clementperon]

@Dimi1010 @seladb IMO it's easier to handle this on the user side based on the cmake version or PKG config version no ?

[seladb]

@Dimi1010 @seladb IMO it's easier to handle this on the user side based on the cmake version or PKG config version no ?

@clementperon But it will make PcapPlusPlus not backward compatible which will make it hard for users to upgrade...

The question is more:

"Do we want the flag to be there to allow people to disable the old includes, or do we want them to be always on until we remove them?"

@Dimi1010 I think having the flag is a good idea. We can leave this flag turned on by default in the next version, then in later version change the default to OFF and eventually remove it altogether

[Dimi1010]

@seladb @clementperon Added the flag PCAPPP_ENABLE_OLD_STYLE_INCLUDE (default ON) to both the root CMakeLists.txt and PcapPlusPlusConfig.cmake. It will also print a deprecation warning if it is turned to ON.

Also can someone look at the android CI and see what is going wrong there. :/

[clementperon]

@Dimi1010 @seladb before breaking we can allow user to use both include style with this PR it will also fix my issue when Pcap++ is installed with Homebrew on MacOS https://github.com/seladb/PcapPlusPlus/pull/1789

[seladb]

This is an old PR that has a lot of conflicts with the current dev branch. I'll close it for now