Skip to content

Buildsystem fixes #3390

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

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

Successfully merging this pull request may close these issues.

2 participants