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

Add JavaCPP Preset for ModSecurity library #1012

Merged
merged 30 commits into from
Apr 11, 2021

Conversation

artemMartynenko
Copy link
Contributor

No description provided.

modsecurity/cppbuild.sh Outdated Show resolved Hide resolved
modsecurity/platform/pom.xml Outdated Show resolved Hide resolved
modsecurity/platform/pom.xml Show resolved Hide resolved
@junlarsen
Copy link
Member

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

@artemMartynenko
Copy link
Contributor Author

artemMartynenko commented Feb 26, 2021

I need some help. I added workflow and now everything works ok for Linux builds, but for macOS, I got a fail class modsecurity is public, should be declared in a file named ModSecurity.java. You can see it in the check result. What can be the reason?

@saudet
Copy link
Member

saudet commented Feb 26, 2021

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.

modsecurity/README.md Outdated Show resolved Hide resolved
@saudet
Copy link
Member

saudet commented Mar 2, 2021

If you could also commit the files under src/gen, it would help to diagnose this issue. Thanks

@artemMartynenko
Copy link
Contributor Author

Sorry for the delay. I was a little bit busy. Now everything works.

Copy link
Member

@junlarsen junlarsen left a 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

@artemMartynenko
Copy link
Contributor Author

Totally agree. Done.

cd ModSecurity
git checkout origin/v3/master
git submodule init
git submodule update
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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 👍

@artemMartynenko
Copy link
Contributor Author

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?

@junlarsen
Copy link
Member

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

@saudet
Copy link
Member

saudet commented Mar 26, 2021

Yes, but also the release version of ModSecurity itself. It's not "3.0.4", so "master" will do for now.

@artemMartynenko
Copy link
Contributor Author

Versions are fixed. Thanks!

# Conflicts:
#	cppbuild.sh
<dependency>
<groupId>org.bytedeco</groupId>
<artifactId>modsecurity-platform</artifactId>
<version>3.0.4-1.5.5-SNAPSHOT</version>
Copy link
Member

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.

Copy link
Member

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...

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

@artemMartynenko artemMartynenko Apr 5, 2021

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

@saudet saudet merged commit 7996aeb into bytedeco:master Apr 11, 2021
@saudet
Copy link
Member

saudet commented Jun 28, 2021

@artemMartynenko FYI, v2.9.4 has now been released. We may want to set the version of the presets to that release.

@artemMartynenko
Copy link
Contributor Author

artemMartynenko commented Jul 6, 2021

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.

@saudet
Copy link
Member

saudet commented Jul 6, 2021

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.

@artemMartynenko
Copy link
Contributor Author

No problem, I will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants