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

Fix CMake #891

Open
wants to merge 71 commits into
base: master
Choose a base branch
from
Open

Fix CMake #891

wants to merge 71 commits into from

Conversation

sgeto
Copy link
Contributor

@sgeto sgeto commented Apr 13, 2018

  • work in progress -

sgeto added 30 commits April 11, 2018 01:06
It's the first file to add and the last one to delete before merger
This will enable us to a lot of useful features.

As far I tell. the only supported OS that comes with a cmake version lower than 3 is Solaris.
However, Solaris users have many ways to upgrade and they really should.

Also updated TODO.CMake.
…_INCLUDE_DIR

...on Windows.

GTK2 support on windows is just as abundant as the FindGTK2.cmake module.
It would take sending a patch upstream or hosting a local copy of FindGTK2.cmake in
cmake/Modules/ to fix it.
Neither of these steps is worth it. We'll look for GTK2 via pkg-config on windows from now.
All GTK2 bundles I know of ship pkg-config. So anyone on windows who wants GTK2, just needs to add
the GTK2 bundle's "bin" directory to their path. Easy peasy.
…dows

This should tell cmake to prefer libs provided in the GTK2 bundle before those
found elsewhere.
FindGTK3.cmake is terrible. 590 lines of confusion. I can already tell that I am
Going to hate that file. We're the only one's maintaining it and it didn't make it
to cmake trunk...

In addition, glibconfig.h is for some reason located under $PREFIX/lib/glib-2.0/include.
I failed to see why. FindGTK3.cmake is unable to locate it and I'm unable to teach
find_path() to look in "lib" directory for header files without using hard-coded pathnames.
Will need to come back to this. Added to TODO.CMake
… default on windows

Turns out that the FindCurses.cmake module in cmake trunk has a bug where it only looks for libform,
even when you request wide-character support. It should look for libformw when CURSES_NEED_WIDE
is true.
That bug is not new. I saw that more than a year ago. Anyway until this is addressed,
we'll host our fixed copy of FindCurses.cmake.

Although these set of commits focus on enhancing cmake, I can already tell that the ncurses UI support
on Windows is promising yet a total mess, and *NOT* my priority.

To summarize:
As stated in include\missing\ncurses_mingw.h:
"OS_WINDOWS doesn't really use the full ncurses yet, but a small layer on-top of PD-curses."

So right now, we have a mix of a old PDcurses library, a local ncurses_mingw.h (which is never included)
some more local defines and functions that are now either part of the PDcurses library (vwprintw), have been
declared deprecated by it (PDC_check_bios_key) and cmake, eagerly looking for ncurses (and ncurses only).

Great.

The bad news is that PDcurses is much more limited then ncurses. The good news is that ncurses support on windows
came a long way. On windows, standard ncurses may very well work. So there may not be any need for PDcurses and any
of these "hacks".
But again, there's much more important things to worry about.
The mcurses interface will be off by default on windows

https://www.gnu.org/software/ncurses/#h4-port-mingw
http://invisible-island.net/ncurses/ncurses.html#download_mingw
It has one job. It can do that in its own .cmake.
Come to think of it: Why are we enforcing an out of source build anyways?
This should be optional...
They are pretty much useless there.
It's not a build-in variable. There are plenty build-ins that provide an equal or better option.
Also used the build-ins FIND_LIBRARY_USE_LIB*_PATHS instead pf hard-coded paths and updated
TODO.CMake.
VERSION wasn't used anywhere anyways...
CMAKE_MODULE_PATH can be set as an enviroment variable by a user to point to
his/her modules.
…INDOWS

The goal is to get ettercap to build on as many toolchains as possible not just OS_MINGW.
OS_MINGW is currently unused and should only be used in situations where a GNU C based
compiler is our only hope.
So it's not "just" a user option and cmake cache variable but also the
the preprocessor definition used in ec_parser.c, ec_set.c etc
If we're targeting some flavor of UN*X or Cygwin (a unix-like environment on
windows with a POSIX-complaint shell and other GNU tools) set INSTALL_SYSCONFDIR
to "/etc", else set it to ${INSTALL_PREFIX}/etc.
That's mostly because there the root directory on those other OSs is not "/" and
it most certainly doesn't hold an "etc" directory.

There is still a lot of room for improvement regarding INSTALL_PREFIX.
See my notes in TODO.CMake.
Why in the world would we ever want to silence all warning?

We also shouldn't blindly assume that every compiler has our flags.
Our custom build flags don't add anything that cmake's default flags don't add.
So why no just using the default?
The macro MacroEnsureOutOfSourceBuild() is now called ensure_out_of_source_build()
and it will share a .cmake file with all other Macros and Functions we come up with called EttercapMacrosAndFunctions.cmake.

Another Macro I added is check_and_add_compiler_option() that we can use to
add all sorts of compile flags without having to worry about whether a build system
supports it.

Speaking of build system:
There is a fundamental difference between makefiles-based generators and IDE-based
generators (aka multi-config generator). IDE-based generators (Visual Studio, xcode,
CodeBlocks etc) have no understanding of CMAKE_BUILD_TYPE. They use CMAKE_CONFIGURATION_TYPES
instead.

So yeah, I made up for that too, and added comments where needed.
Generally speaking, adding debug compile flags is now smarter, cleaner and
more IDE-friendly. Check the diff.
My bad. I also messed up HAVE_GTK3COMPAT.
I'll add them back differently, promise.
I guess it was added before cmake introduced CheckSymbolExists, which
does exactly the same thing, but better.
Actually we still need to employ check_c_source_compiles() to find
PTHREAD_MUTEX_RECURSIVE_NP on Linux because it's an enum there, which kind of
defeats the purpose of the whole HAVE_MUTEX_RECURSIVE_NP exercise...
At least OS_MINGW has nameser.h. Just not in an "arpa" subdirectory.
...to the include directories in every processed CMakeLists.txt.
@koeppea
Copy link
Member

koeppea commented May 3, 2018 via email

sgeto added 5 commits May 3, 2018 13:59
…EttercapFeatureCheck.cmake

As well as moved function checks in EttercapLibCheck.cmake to EttercapFeatureCheck.cmake and used CheckSymbolExists() instead of CheckFunctionExists() wherever appropriate.
It will be a lot and we don't want to spam our top-level CMakeLists.txt
@koeppea
Copy link
Member

koeppea commented Jun 27, 2018

Hey @sgeto. How is it going with this pull request?
Ready for rebase and sqashing?

@koeppea
Copy link
Member

koeppea commented Jun 27, 2018

Travis fails due to CMAKE formatting issue:

./cmake/Modules/FindLUAJIT.cmake:39: Tab found; please use spaces [whitespace/tabs]

@koeppea
Copy link
Member

koeppea commented Jun 27, 2018

Any reason why clang-lint is only executed if the compiler is GCC?:

script:
  - if [[ "$CC" == "gcc" && "${BUILD_ARGS}" = "" ]]; then git clone https://github.com/richq/cmake-lint.git; cd cmake-lint; python setup.py install --user; cd -; ./misc/cmakelint.sh; fi
  - mkdir build
  - cd build
  - cmake -DENABLE_TESTS=On $BUILD_ARGS ..
  - make
  - make test_verbose

@koeppea
Copy link
Member

koeppea commented Jun 27, 2018

Now Travis fails with:

CMake Error at /usr/local/cmake-3.9.2/share/cmake-3.9/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find GEOIP: Found unsuitable version "1.6.0", but required is at
  least "1.6.4" (found /usr/lib/x86_64-linux-gnu/libGeoIP.so)

What was the reason to require version 1.6.4 for GeoIP?

@koeppea
Copy link
Member

koeppea commented Jul 8, 2018

What was the reason to require version 1.6.4 for GeoIP?

@sgeto Any idea? Or can we remove the version prerequisite?

@sgeto
Copy link
Contributor Author

sgeto commented Aug 24, 2018

Hi @koeppea,
could you revert you last few commits, please?

@koeppea
Copy link
Member

koeppea commented Aug 25, 2018

was only one. Removed. HEAD on GitHub is now back at f0d6512

@koeppea
Copy link
Member

koeppea commented Apr 7, 2019

Just lowered the version requirement for GeoIP to see how far Travis gets then...

@koeppea koeppea force-pushed the fix-cmake branch 4 times, most recently from e892781 to 91b1b14 Compare April 18, 2019 19:02
@koeppea
Copy link
Member

koeppea commented Apr 19, 2019

@sgeto I've pushed some commits to your branch and finally got this to pass all Travis test cases.
What do you think?

@LocutusOfBorg
Copy link
Contributor

@sgeto @koeppea hello lets rebase and merge?

@koeppea
Copy link
Member

koeppea commented Nov 29, 2022

Though I like the content and concepts of this pull-request it's meanwhile quite outdated.
But my CMake capabilities are not sufficient to bring this PR back to a up2date state.
Have tried to reach out to @sgeto hoping he is able to continue and finalize his work he started in 2018.

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