Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
11fcea2
3f35394
2d5a797
5bd4596
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
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.amI 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.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