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

First step of cleaning up the Makefile.in hell. #1163

Closed
wants to merge 1 commit into from

Conversation

utoni
Copy link
Collaborator

@utoni utoni commented Apr 6, 2021

Before fixing the protocol detection issues, there is another issue which annoys me since I started using nDPI.
Until this PR, out-of-source builds were impossible due to broken autogen.sh and Makefile.in's.
To "bypass" this limitations, multiple git worktree's were required.
Having half-a-dozens additional nDPI directories just to build the library with different toolchains is not a good idea.
That PR is the first step addressing this issue.
I've tried to test as much as I could, but three things are currently untested:

  • nDPI w/ DPDK build
  • homebrew package build
  • rpm package build

I urge everyone to use Makefile.am (Automake) instead of Makefile.in (which just gets processed by Autoconf).
The Makefile.am's can be generated with utils/generate_automake_am.sh e.g. when a new tests/pcap or a src/ file has been added, but can also be added manually to Makefile.am.

The goal is to get rid of the Makefile.in's (replace it with Automake Makefile.am's)
as they duplicate lot's of text.
That decreases readability and is in general a bad design pattern.
It seems appropriate to use Automake for an Autoconf based project.

Currently achieved:

  • using libtool to build the core library (+libtool's semantic versioning)
  • out-of-source builds should work right now
  • introducing Automake based Makefile in src/

Signed-off-by: Toni Uhlig matzeton@googlemail.com

@utoni utoni force-pushed the first-step-to-automake-integration branch 3 times, most recently from 6c67aef to 1b7be43 Compare April 6, 2021 16:11
@lucaderi
Copy link
Member

lucaderi commented Apr 7, 2021

I like your contribution but the idea is to simplify rather than to make things less flexible. For instance I see you have listed in the makefile.am all the files. Makefile and llibtool are problems in some platforms (e.g. OpenWRT) and I would like to avoid them as much as possible.
@IvanNardi What do you think?

@IvanNardi
Copy link
Collaborator

@IvanNardi What do you think?

I am not an expert of Makefiles (or build systems in general)  and I consider the autoconf stuff some kind of "dark magic" completely out of my league. Therefore I can't give any proper advice on this matter.
Being able to compile nDPI out-of-tree is a very nice feature; to keep compiling nDPI in all the used architectures is quite important, though
Sorry to not be more useful

@IvanNardi
Copy link
Collaborator

@lnslbrty, I briefly tested your PR, everything seems to work (for my very, very basic workflows, at least) but I have a question.
With the current master, I usually compile nDPI via "make -s" to have a very clean output and to easily detect any (new) warnings; is it possible to have a similar output even with your changes?
Please confront

ivan@ivan-Latitude-E6540:~/svnrepos/nDPI(dev)$ make -s
Making all in src/lib
Making all in example
ar: creating libndpiReader.a
a - reader_util.o
a - intrusion_detection.o
Making all in tests
Making all in tests/unit
Making all in tests/dga
ivan@ivan-Latitude-E6540:~/svnrepos/nDPI(dev)$

with

ivan@ivan-Latitude-E6540:~/svnrepos/nDPI/build(pr-1163)$ make -s
Making all in src
libtool: compile:  clang-11 -DHAVE_CONFIG_H -I. -I../../src -I../src/include -fPIC -DPIC -I../../src/include -I../../src/lib -I../../src/lib/third_party/include -DNDPI_LIB_COMPILATION -Wall -W -Wall -Wno-unused-parameter -Wno-sign-compare -g -O3 -g3 -O0 -Wno-unused-value -fsanitize=address -fsanitize=undefined -fno-sanitize=alignment -fsanitize=leak -fno-omit-frame-pointer -W -Wall -Wno-unused-parameter -Wno-sign-compare -g -O3 -g3 -O0 -Wno-unused-value -fsanitize=address -fsanitize=undefined -fno-sanitize=alignment -fsanitize=leak -fno-omit-frame-pointer -MT lib/libndpi_la-ndpi_analyze.lo -MD -MP -MF lib/.deps/libndpi_la-ndpi_analyze.Tpo -c ../../src/lib/ndpi_analyze.c  -fPIC -DPIC -o lib/.libs/libndpi_la-ndpi_analyze.o
libtool: compile:  clang-11 -DHAVE_CONFIG_H -I. -I../../src -I../src/include -fPIC -DPIC -I../../src/include -I../../src/lib -I../../src/lib/third_party/include -DNDPI_LIB_COMPILATION -Wall -W -Wall -Wno-unused-parameter -Wno-sign-compare -g -O3 -g3 -O0 -Wno-unused-value -fsanitize=address -fsanitize=undefined -fno-sanitize=alignment -fsanitize=leak -fno-omit-frame-pointer -W -Wall -Wno-unused-parameter -Wno-sign-compare -g -O3 -g3 -O0 -Wno-unused-value -fsanitize=address -fsanitize=undefined -fno-sanitize=alignment -fsanitize=leak -fno-omit-frame-pointer -MT lib/libndpi_la-ndpi_analyze.lo -MD -MP -MF lib/.deps/libndpi_la-ndpi_analyze.Tpo -c ../../src/lib/ndpi_analyze.c -o lib/libndpi_la-ndpi_analyze.o >/dev/null 2>&1
[...]

Thanks

@lucaderi
Copy link
Member

On my system libtool/automake... are making compilation much slower and complex. I am still thinking about this PR if it makes sense or not. I am not a make expert but I believe cmake is the way to go it we want to change makefiles. @lnslbrty What do you think ?

@utoni
Copy link
Collaborator Author

utoni commented Apr 10, 2021

@IvanNardi
Adding LIBTOOLFLAGS=--silent to src/Makefile.am disables "chatty" libtool and make -s should now work as expected.

@lucaderi
I aggree regarding compilation times (performance penalty of about ~2x slower). This is due to the bloated libtool script, which gets executed for each compilation unit. But I think that should not be a real issue for projects as only those units with changed sources/headers should be recompiled. So only the initial compilation makes a huge difference.

Another solution to achieve working out-of-source builds could be to fix all Makefile.in's so they are "path agnostic". But I'm unsure if that is a better solution. For example: When I started cleaning up some of the messy Makefile.in's I recognized fixing one caused build issues for others as they were now relying on non existent build targets/paths. Some of those were also copy&pasted, meaning they did contain unused variables and duplicated code is in general harder to maintain.

Regarding OpenWrt: I was able to compile it w/o issues.

@utoni
Copy link
Collaborator Author

utoni commented Apr 10, 2021

This PR contains also an important fix I did not mention in the PR description.
Before that PR, all executables and libraries were linked against libjson-c.
But only tests/unit required it.
This is bad for all package maintainers e.g. me for OpenWrt as libndpi.so required libjson-c.so but does not use it at all.

@IvanNardi
Copy link
Collaborator

@IvanNardi
Adding LIBTOOLFLAGS=--silent to src/Makefile.am disables "chatty" libtool and make -s should now work as expected.

@lnslbrty , I confirm LIBTOOLFLAGS=--silent works as expected! Thanks!

@utoni utoni force-pushed the first-step-to-automake-integration branch 2 times, most recently from 52e182e to 78dc903 Compare April 15, 2021 08:14
@utoni utoni force-pushed the first-step-to-automake-integration branch 2 times, most recently from e3fa995 to fd695c4 Compare May 10, 2021 11:51
@utoni utoni force-pushed the first-step-to-automake-integration branch from fd695c4 to 70f0350 Compare May 19, 2021 19:06
@utoni utoni force-pushed the first-step-to-automake-integration branch from 70f0350 to a11ab8a Compare June 9, 2021 16:26
@utoni utoni force-pushed the first-step-to-automake-integration branch from a11ab8a to b7a9346 Compare June 23, 2021 08:38
@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@utoni utoni force-pushed the first-step-to-automake-integration branch from b7a9346 to 613a80a Compare July 30, 2021 13:02
The goal is to get rid of the Makefile.in's (replace it with Automake Makefile.am's)
as they duplicate lot's of text.
That decreases readability and is in general a bad design pattern.
It seems appropriate to use Automake for an Autoconf based project.

Currently achieved:

 * using libtool to build the core library (+libtool's semantic versioning)
 * out-of-source builds should work right now
 * introducing Automake based Makefile in src/
 * removed some (maybe) unused GIT ignored files
 * provide some small python fixes

Signed-off-by: Toni Uhlig <matzeton@googlemail.com>
@utoni utoni force-pushed the first-step-to-automake-integration branch from 613a80a to b503900 Compare July 30, 2021 20:09
@sonarcloud
Copy link

sonarcloud bot commented Jul 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lucaderi
Copy link
Member

@lnslbrty I will put this PR on hold also as many things have changed and it won't merge. I am open to improvements but not to put automake everywhere that slows down compilation and crates challenges on building on some embedded platforms.

@lucaderi lucaderi closed this Sep 22, 2021
utoni added a commit to utoni/nDPI that referenced this pull request May 25, 2022
This fixes some build/test issues resulting when using tarballs.

 * nDPI uses autotools (especially autoconf) in a wrong way, see ntop#1163

Signed-off-by: lns <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 25, 2022
This fixes some build/test issues resulting when using tarballs.

 * nDPI uses autotools (especially autoconf) in a wrong way, see ntop#1163

Signed-off-by: lns <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 26, 2022
This fixes some build/test issues resulting when using tarballs.

 * nDPI uses autotools (especially autoconf) in a wrong way, see ntop#1163

Signed-off-by: lns <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 26, 2022
This fixes some build/test issues resulting when using tarballs.

 * nDPI uses autotools (especially autoconf) in a wrong way, see ntop#1163

Signed-off-by: lns <matzeton@googlemail.com>
utoni added a commit to utoni/nDPI that referenced this pull request May 26, 2022
This fixes some build/test issues resulting when using tarballs.

 * nDPI uses autotools (especially autoconf) in a wrong way, see ntop#1163

Signed-off-by: lns <matzeton@googlemail.com>
utoni added a commit that referenced this pull request May 26, 2022
This fixes some build/test issues resulting when using tarballs.

 * nDPI uses autotools (especially autoconf) in a wrong way, see #1163

Signed-off-by: lns <matzeton@googlemail.com>
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.

3 participants