Skip to content

Buildsystem fixes #3390

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

Open
wants to merge 4 commits into
base: v3/master
Choose a base branch
from

Conversation

arvedarved
Copy link

@arvedarved arvedarved commented May 22, 2025

what

  • These changes fix the build for srcdir != builddir. At several points in the buildsystem relative (to the Makefile) paths were used. this does not work if a clean builddir is used, as the Makefile and generated headers reside in the builddir. auttools provide the variables $(top_srcdir) and $(top_builddir) to refer to the srcdir and builddir respectively

  • If libxml2 is installed in a non-default location linking to libxml2 fails due to a missing LIBXML2_LDFLAGS

  • autotools don't support wildcards (see link to documentation below).

  • Note: Because these fixes use a GNU make extension, the build for non-GNU make is broken. If non-GNU make is supported by modsecurity, most likely the files need to be added explicitly. automake warns about this portability, thats why Werror was removed from automake options

  • Note2: I only fixed my configuration, there might be other optional parts of modsecurity, that make the same assumptions, which I did not touch.

why

references

Tilman Keskinöz added 3 commits May 22, 2025 18:59
If libxml2 is in a non-default directory, it needs to be added
to LDFLAGS
automake doesn't support wildcards.
See https://www.gnu.org/software/automake/manual/html_node/Wildcards.html
for details.

Use the GNU make $(wildcard ) extension.

Note: this breaks with non-GNU make
@airween
Copy link
Member

airween commented May 22, 2025

Hi @arvedarved,

first of all, thank you for your first contribution! 🎉

Unfortunately there are a few issues in build processes in case of Linux and MacOS.

Like this one - could you take a look at them?

Thanks!

Reported by: CI via Ervin Hegedus
Copy link

@arvedarved
Copy link
Author

Hi Ervin, Thanks for looking at the pull request. Indeed the change in the test directory looks dubious, I tried to fix it.
I assume the regression tests are run automatically? If not could you trigger them again?

@airween
Copy link
Member

airween commented May 23, 2025

Hi @arvedarved,

I assume the regression tests are run automatically? If not could you trigger them again?

yes, the regression tests run automatically - normally. But while this is your first contribution, I have to approve the run. If you're done with it, and the PR will be merged, then your PR's will start the tests automatically.

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.

2 participants