-
Notifications
You must be signed in to change notification settings - Fork 744
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
Add JavaCPP Preset for ModSecurity library #1012
Conversation
remove unsupported platform builds
We should also set up automatic builds for the preset through GitHub Actions like we have for the other presets, see https://github.com/bytedeco/javacpp-presets/tree/master/.github/workflows |
I need some help. I added workflow and now everything works ok for Linux builds, but for macOS, I got a fail |
File names are case insensitive on Mac and Windows. We can't have both a file named "modsecurity.java" and "ModSecurity.java" in the same directory. |
If you could also commit the files under |
Sorry for the delay. I was a little bit busy. Now everything works. |
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 believe modsecurity should be added to this list in the main cppbuild.sh script
https://github.com/bytedeco/javacpp-presets/blob/master/cppbuild.sh#L167
Totally agree. Done. |
modsecurity/cppbuild.sh
Outdated
cd ModSecurity | ||
git checkout origin/v3/master | ||
git submodule init | ||
git submodule update |
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.
They seem to offer proper source code bundles for releases:
https://github.com/SpiderLabs/ModSecurity/releases
Say we replace these git
calls with something like the following, does it still work?
download https://github.com/SpiderLabs/ModSecurity/releases/download/v3.0.4/modsecurity-v3.0.4.tar.gz modsecurity-v3.0.4.tar.gz
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 need to check it. I will answer as soon as check it.
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 checked it. Unfortunately, it not works. There were a lot of changes from the previous release and it seems that release rules will be changed. I think it will work after the release 3.1.0. And also their current recommendation to build from the master branch, maybe it associated with the release policy.
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.
Hum, ok, let's wait and see for 3.1.0? How soon is that going to happen?
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.
We could also set the version of the presets to master
, temporarily, until 3.1.0 gets released. What do you think?
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.
Yep, we can set the version of the preset to master, temporarily, and then change it to 3.0.5 after the release. "Migration from master and 3.0.5 to 3.1.0 will be much easier" - it`s a comment which I got on the issue about next release. Thanks a lot 👍
I have seen that 1.5.5 was released, so it seems that I need to update the code in PR to 1.5.6-SNAPSHOT to make it acceptable? |
Correct, the version should be changed to 1.5.6-SNAPSHOT |
Yes, but also the release version of ModSecurity itself. It's not "3.0.4", so "master" will do for now. |
# Conflicts: # cppbuild.sh
Versions are fixed. Thanks! |
# Conflicts: # cppbuild.sh
modsecurity/README.md
Outdated
<dependency> | ||
<groupId>org.bytedeco</groupId> | ||
<artifactId>modsecurity-platform</artifactId> | ||
<version>3.0.4-1.5.5-SNAPSHOT</version> |
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 versions haven't been updated in the README.md file.
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 still see 3.0.4 in the README.md...
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.
maybe you did not update? I fixed it with the last commit today.
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.
No, still seeing it in the update: https://github.com/bytedeco/javacpp-presets/blob/045c7f5e114509fd442e0a34f5f5287bb33daa24/modsecurity/README.md#introduction
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.
Sorry, I focused on commented lines. Now it`s already fixed. Thanks!
brew install boost ccache gcc swig autoconf-archive automake cmake libomp libtool libusb ant maven nasm xz pkg-config sdl gpg1 bison flex perl ragel binutils gradle gmp isl libmpc mpfr | ||
brew install boost ccache gcc swig autoconf-archive automake cmake libomp libtool libusb ant maven nasm xz pkg-config sdl gpg1 bison flex perl ragel binutils gradle gmp isl libmpc mpfr zlib curl pcre libffi yajl ssdeep luarocks | ||
brew install geoip --with-geoipupdate | ||
brew install doxygen --with-llvm |
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.
Why is the set of dependencies different on Linux and Mac? It looks like it's building without any of those dependencies too. Which ones do you think we actually need?
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 you are talking about geoip and doxygen they are installing on Linux too. For Mac, they have separate lines just because they need additional args for installation. Or you meant 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.
Well, it doesn't look like Lua or libffi gets used for anything, while curl and zlib come with the system, so I'll probably just leave those out. I also don't think we need to geoipupdate or LLVM in there either. Sounds good?
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'm also not sure we need doxygen to build an installation of the project, see https://github.com/SpiderLabs/ModSecurity#Dependencies
Doxygen is used to create api documentation from the source code as well as other documentation pages. Shouldn't be necessary to build modsecurity.
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.
Sorry for the late response. @saudet yes it sounds good. I see that you already fixed it. Thanks! Once again sorry for the late response, I was AFK last week.
@artemMartynenko FYI, v2.9.4 has now been released. We may want to set the version of the presets to that release. |
Hello. You should not set the version to 2.9.4 because it's a supporting release of the previous version. This preset that were merged it`s for version 3+. We set the version to master because there were intermediate changes between 3.0.4 and 3.0.5. I got the message that version 3.0.5 will be released tomorrow, so after release, we can fix the version to 3.0.5. |
Thanks! Please send a pull request when it's released. |
No problem, I will do that. |
No description provided.