Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support the Wireshark plugin for more Wireshark distributions #183

Merged

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Jan 31, 2023

Motivation

#182 fixes the Wireshark build process on macOS. However, it breaks the compatibility with some Wireshark distributions like the default Wireshark 3.2.3 on Ubuntu 20.04. The reason is some Wireshark distributions use config.h to record the versions, while other might use ws_version.h.

See
https://listman.redhat.com/archives/libvir-list/2020-September/msg00377.html for a similar fix.

Modifications

Try to find the ws_version.h first, if it's not found, find the config.h.

To be more user friendly:

  • Separate the dissector from the whole project so that we can build the dissector without building the Pulsar C++ client.
  • Refactor the README of the Wireshark dissector by focusing on how to use.

Verifications

Add a workflow to build Wireshark dissector on both Ubuntu and macOS.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@BewareMyPower BewareMyPower added documentation Improvements or additions to documentation build labels Jan 31, 2023
@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Jan 31, 2023
@BewareMyPower BewareMyPower self-assigned this Jan 31, 2023
@BewareMyPower BewareMyPower force-pushed the bewaremypower/wireshark-fix branch 2 times, most recently from 1486ce6 to f054988 Compare January 31, 2023 12:49
@BewareMyPower
Copy link
Contributor Author

@Demogorgon314 Could you help verify this CMakeLists.txt work on macOS? Not sure if -Wl,-all_load could be removed.

@Demogorgon314
Copy link
Member

I tried it on macOS, and it has a compile error.

➜  wireshark git:(bewaremypower/wireshark-fix) ✗ cmake -B build -DWIRESHARK_INCLUDE_PATH=/opt/homebrew/Cellar/wireshark/4.0.3/include
-- Use WIRESHARK_INCLUDE_PATH: /opt/homebrew/Cellar/wireshark/4.0.3/include
-- Use GLIB_INCLUDE_DIRS: /opt/homebrew/Cellar/glib/2.74.5/include/glib-2.0;/opt/homebrew/Cellar/glib/2.74.5/lib/glib-2.0/include;/opt/homebrew/opt/gettext/include;/opt/homebrew/Cellar/pcre2/10.42/include
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/wangkai/Developer/github/pulsar-client-cpp/wireshark/build

➜  wireshark git:(bewaremypower/wireshark-fix) ✗ cmake --build build                                                                 
[ 25%] Building CXX object CMakeFiles/pulsar-dissector.dir/pulsarDissector.cc.o
/Users/wangkai/Developer/github/pulsar-client-cpp/wireshark/pulsarDissector.cc:21:1: error: unknown type name 'constexpr'
constexpr int kWiresharkMajorVersion = WIRESHARK_VERSION_MAJOR;
^
/Users/wangkai/Developer/github/pulsar-client-cpp/wireshark/pulsarDissector.cc:22:1: error: unknown type name 'constexpr'
constexpr int kWiresharkMinorVersion = WIRESHARK_VERSION_MINOR;
^
In file included from /Users/wangkai/Developer/github/pulsar-client-cpp/wireshark/pulsarDissector.cc:37:
/Users/wangkai/Developer/github/pulsar-client-cpp/wireshark/build/generated/lib/PulsarApi.pb.h:10:10: fatal error: 'google/protobuf/port_def.inc' file not found
#include <google/protobuf/port_def.inc>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.
gmake[2]: *** [CMakeFiles/pulsar-dissector.dir/build.make:83: CMakeFiles/pulsar-dissector.dir/pulsarDissector.cc.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/pulsar-dissector.dir/all] Error 2
gmake: *** [Makefile:91: all] Error 2

And if I don't specify the WIRESHARK_INCLUDE_PATH, it cannot find the path automatically.

-- Cannot find ws_version.h, fallback to find config.h
CMake Error at CMakeLists.txt:31 (message):
  Failed to find WIRESHARK_INCLUDE_PATH


-- Configuring incomplete, errors occurred!

@BewareMyPower
Copy link
Contributor Author

I see. Mark it as drafted now. I will fix it soon.

@BewareMyPower BewareMyPower marked this pull request as draft January 31, 2023 14:06
wireshark/README.md Outdated Show resolved Hide resolved
wireshark/README.md Outdated Show resolved Hide resolved
@BewareMyPower BewareMyPower force-pushed the bewaremypower/wireshark-fix branch from 7122b07 to 9302d3a Compare February 2, 2023 08:22
@BewareMyPower BewareMyPower marked this pull request as ready for review February 2, 2023 08:23
@BewareMyPower
Copy link
Contributor Author

@Anonymitaet The comments have been addressed now.

@Demogorgon314 I've fixed the build failure and added the workflow to build the Wireshark plugin on macOS, could you also verify whether it could be loaded?

@Demogorgon314
Copy link
Member

@BewareMyPower Now it can build success and load, but it still needs manually specific WIRESHARK_INCLUDE_PATH.

@BewareMyPower
Copy link
Contributor Author

@Demogorgon314 It's confirmed that the install path /opt/homebrew/Cellar/wireshark/4.0.3/include on macOS M1 cannot be found by CMake directly. I've added back the document for users to specify the installation path of Wireshark.

### Motivation

apache#182 fixes the Wireshark
build process on macOS. However, it breaks the compatibility with some
Wireshark distributions like the default Wireshark 3.2.3 on Ubuntu
20.04. The reason is some Wireshark distributions use `config.h` to
record the versions, while other might use `ws_version.h`.

See
https://listman.redhat.com/archives/libvir-list/2020-September/msg00377.html
for a similar fix.

### Modifications

Try to find the `ws_version.h` first, if it's not found, find the
`config.h`. Add the workflow to verify it can be built on Ubuntu.

To be more user friendly:
- Separate the dissector from the whole project so that we can build the
  dissector without building the Pulsar C++ client.
- Refactor the README of the Wireshark dissector by focusing on how to
  use.

### Verifications

Add a workflow to build Wireshark dissector on both Ubuntu and macOS.
@BewareMyPower BewareMyPower force-pushed the bewaremypower/wireshark-fix branch from 107b59c to 23323eb Compare February 2, 2023 10:38
@Anonymitaet
Copy link
Member

Thanks! Now the docs LGTM.

@BewareMyPower BewareMyPower merged commit 63c4245 into apache:main Feb 3, 2023
@BewareMyPower BewareMyPower deleted the bewaremypower/wireshark-fix branch February 5, 2023 15:18
BewareMyPower added a commit that referenced this pull request Feb 8, 2023
* Support the Wireshark plugin for more Wireshark distributions

### Motivation

#182 fixes the Wireshark
build process on macOS. However, it breaks the compatibility with some
Wireshark distributions like the default Wireshark 3.2.3 on Ubuntu
20.04. The reason is some Wireshark distributions use `config.h` to
record the versions, while other might use `ws_version.h`.

See
https://listman.redhat.com/archives/libvir-list/2020-September/msg00377.html
for a similar fix.

### Modifications

Try to find the `ws_version.h` first, if it's not found, find the
`config.h`. Add the workflow to verify it can be built on Ubuntu.

To be more user friendly:
- Separate the dissector from the whole project so that we can build the
  dissector without building the Pulsar C++ client.
- Refactor the README of the Wireshark dissector by focusing on how to
  use.

### Verifications

Add a workflow to build Wireshark dissector on both Ubuntu and macOS.

(cherry picked from commit 63c4245)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build cherry-picked/3.1 documentation Improvements or additions to documentation release/3.1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants