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.

[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
#1789