-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
doc: update eBPF/compilation instructions v5 #10840
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
python3-yaml rustc zlib1g-dev | ||
# install-guide-documentation tag end: Recommended dependencies | ||
|
||
- run: CFLAGS="${DEFAULT_CFLAGS}" ./configure --enable-ebpf --enable-ebpf-build |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will change
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this 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
There was a problem hiding this 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.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will change
Continues in #11176 |
Follow-up of #10788
Link to Redmine tickets:
Describe changes:
v5
v4
v3
v2
v1