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

doc: update eBPF/compilation instructions v5 #10840

Closed

Conversation

lukashino
Copy link
Contributor

Follow-up of #10788

Link to Redmine tickets:

Describe changes:
v5

  • inlined and shortened PATH variable edits in the RPM section
  • added minimal/recommended builds to the GH workflow from Ubuntu and Almalinux
  • added literalincludes to the docs from the Github builds
  • removed gosu from Github builds
  • added DPDK to the list of recommended dependencies

v4

  • inlined and shortened PATH variable edits in the Rust section
  • can be cloned - be added

v3

  • updates to the docs per Jeff's suggestions
  • removed the instruction to install i386 headers for the eBPF

v2

  • remove recommended configure parameters

v1

  • small edit in eBPF instructions to prefer libbpf-dev package to the cloning&compilation
  • porting changes from the improved version of installation instructions that got merged into master-6.0.x

Copy link

codecov bot commented Apr 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.86%. Comparing base (784ce30) to head (5bd4596).
Report is 127 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10840      +/-   ##
==========================================
+ Coverage   82.83%   82.86%   +0.03%     
==========================================
  Files         913      913              
  Lines      246847   246847              
==========================================
+ Hits       204474   204557      +83     
+ Misses      42373    42290      -83     
Flag Coverage Δ
fuzzcorpus 64.37% <ø> (+0.06%) ⬆️
suricata-verify 62.09% <ø> (+<0.01%) ⬆️
unittests 62.33% <ø> (ø)

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

sudo apt-get install libnetfilter-queue-dev libnetfilter-queue1 \
libnetfilter-log-dev libnetfilter-log1 \
libnfnetlink-dev libnfnetlink0
.. literalinclude:: ../../.github/workflows/builds.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm realizing now that we may not put these .github files in our release tarballs, so what happens when the .github dir isn't available?

Copy link
Contributor Author

@lukashino lukashino Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that depends whether the docs are generated before or after the .github files are removed.
If the docs are generated prior to that then it is all fine because the literal text includes are already part of the docs.
If the docs are generated past .github folder removal then it will be missing and a warning will be generated during the doc build process.

I've tried to look into how we could substitute the missing text with a placeholder in case .github folder will be missing. Unfortunately, there is no such option atm. We could extend the literalinclude with our custom version.
Or we can again reverse the logic and use something similar approach but from .github. The build defined in .github will clone the Suricata repository, fetch the part of the documentation in between the tags and then execute it as a command.
But that is something we can think of if building the docs will be a problem in the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't make distcheck show this, then, if a problem exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would, that's why I had to add .github/workflows/builds.yml \ to Makefile.am
I was not sure if that also applies to the case when we package Suricata but if it does then that's even better :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember make distcheck being the most annoying and strict in terms of doc generation, when I was bringing the devguide into the userguide, and that we use that to check if Suri packages are generated properly. So I imagine that it would. 🤔 But not sure if we must double-check something else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our CI only does distcheck's from Git. A requirement of the dist is generating the documentation.

I bet if you re-did a distcheck from the output of a distcheck it might not work, as this ~/.github/ directory is not there.

So we should only rely on files that make it into the dist for the dist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this PR updates the Makefile to include .github/workflows/builds.yml into the release.

@catenacyber catenacyber added the typo/doc update No code change : only doc or typo fixes label Apr 18, 2024
python3-yaml rustc zlib1g-dev
# install-guide-documentation tag end: Recommended dependencies

- run: CFLAGS="${DEFAULT_CFLAGS}" ./configure --enable-ebpf --enable-ebpf-build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think ebpf is recommended. If it is, it should be enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to include recommended dependencies then it would make sense to continuously test if we can actually compile Suricata with the recommended dependencies for the recommended features. If we go with the minimal dependency only then this doesn't make sense indeed.

or the cbindgen version is lower than required, it can be
alternatively installed as: cargo install --force cbindgen
3) Make sure the cargo path is within your PATH environment
echo 'export PATH+=":~/.cargo/bin"' >> ~/.bashrc && export PATH+=":~/.cargo/bin"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds the path to the end, typically you want this before system ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will change

Comment on lines +1419 to +1425
sudo dnf install -y autoconf automake clang diffutils dpdk-devel \
file-devel hiredis-devel hyperscan-devel jansson-devel libbpf-devel \
libcap-ng-devel libevent-devel libmaxminddb-devel libnet-devel \
libnetfilter_queue-devel libnfnetlink-devel libpcap-devel \
libyaml-devel llvm-toolset lua-devel lz4-devel make \
nspr-devel nss-devel pcre2-devel pkgconfig python3-devel \
python3-sphinx python3-yaml zlib-devel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove clang, hiredis-devel, libeven-devel, libmaxminddb-devel, is llvm-toolset needed? nspr-devel, nss-devel can go as well. Python3-sphinx shouldn't be there for end-user dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang, hiredis-devel, libeven-devel, libmaxminddb-devel, is llvm-toolset needed? nspr-devel, nss-devel can go as well

The original "recommended dependencies" section contains these dependencies. While we don't pass any flags to configure, I though the purpose of the recommended step was to install all the recommended dependencies to be ready for most of the other configure flags that a user might want to enable.

If we e.g. remove clang, maxminddb, etc. then you could possibly just mention the minimal compilation dependency requirements and for other stuff let the user find the relevant sections of Suricata docs to install the required dependencies. (so e.g. if the user wants to include eBPF, the user searches for eBPF docs and from there, the dependencies and other instructions can be taken). This would mean that OISF is not favoring any other dependency except the required ones.

Is this something that you are aiming for?

@catenacyber catenacyber added the needs rebase Needs rebase to master label Apr 30, 2024
Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some changes were requested and a rebase is at least needed

Copy link
Contributor Author

@lukashino lukashino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next PR I will try to remove recommended section from the docs and I will only include the minimal requirements.

Comment on lines +1419 to +1425
sudo dnf install -y autoconf automake clang diffutils dpdk-devel \
file-devel hiredis-devel hyperscan-devel jansson-devel libbpf-devel \
libcap-ng-devel libevent-devel libmaxminddb-devel libnet-devel \
libnetfilter_queue-devel libnfnetlink-devel libpcap-devel \
libyaml-devel llvm-toolset lua-devel lz4-devel make \
nspr-devel nss-devel pcre2-devel pkgconfig python3-devel \
python3-sphinx python3-yaml zlib-devel
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang, hiredis-devel, libeven-devel, libmaxminddb-devel, is llvm-toolset needed? nspr-devel, nss-devel can go as well

The original "recommended dependencies" section contains these dependencies. While we don't pass any flags to configure, I though the purpose of the recommended step was to install all the recommended dependencies to be ready for most of the other configure flags that a user might want to enable.

If we e.g. remove clang, maxminddb, etc. then you could possibly just mention the minimal compilation dependency requirements and for other stuff let the user find the relevant sections of Suricata docs to install the required dependencies. (so e.g. if the user wants to include eBPF, the user searches for eBPF docs and from there, the dependencies and other instructions can be taken). This would mean that OISF is not favoring any other dependency except the required ones.

Is this something that you are aiming for?

python3-yaml rustc zlib1g-dev
# install-guide-documentation tag end: Recommended dependencies

- run: CFLAGS="${DEFAULT_CFLAGS}" ./configure --enable-ebpf --enable-ebpf-build
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to include recommended dependencies then it would make sense to continuously test if we can actually compile Suricata with the recommended dependencies for the recommended features. If we go with the minimal dependency only then this doesn't make sense indeed.

or the cbindgen version is lower than required, it can be
alternatively installed as: cargo install --force cbindgen
3) Make sure the cargo path is within your PATH environment
echo 'export PATH+=":~/.cargo/bin"' >> ~/.bashrc && export PATH+=":~/.cargo/bin"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will change

@lukashino
Copy link
Contributor Author

Continues in #11176

@lukashino lukashino closed this May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master typo/doc update No code change : only doc or typo fixes
Development

Successfully merging this pull request may close these issues.

5 participants