Conversation
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
MishimaHaruna
left a comment
MishimaHaruna
left a comment
There was a problem hiding this comment.
Initial review - will continue later (I'm going commit by commit, so some comments might be outdated, feel free to close them)
|
|
||
| include(CMakeDependentOption) | ||
| # Configuration options | ||
| cmake_dependent_option(ENABLE_LIBBACKTRACE "Compiles with libbacktrace. (off by default - experimental)" ON ${RUNNING_ON_LINUX} OFF) |
There was a problem hiding this comment.
I think it's more typical to express this as
-cmake_dependent_option(ENABLE_LIBBACKTRACE "Compiles with libbacktrace. (off by default - experimental)" ON ${RUNNING_ON_LINUX} OFF)
+cmake_dependent_option(ENABLE_LIBBACKTRACE "Compiles with libbacktrace. (off by default - experimental)" ON RUNNING_ON_LINUX OFF)(let the condition parser expand the variable, so that if it were to contain weird stuff, it wouldn't break)
|
|
||
| if(NOT "${CMAKE_BUILD_TYPE}" STREQUAL "Debug") | ||
| if(${ENABLE_GDB}) | ||
| message(FATAL_ERROR "ENABLE_GDB can be only used in debug builds") |
There was a problem hiding this comment.
I think it might make sense to allow this in RelWithDebInfo as well
| get_directory_property(SYSGEN_CFLAGS COMPILE_OPTIONS) | ||
| get_directory_property(SYSGEN_DEFS COMPILE_DEFINITIONS) | ||
|
|
||
| add_custom_command( |
There was a problem hiding this comment.
OS-specific, will need to be put behind a condition
| if(CMAKE_SIZEOF_VOID_P EQUAL 4) | ||
| message(FATAL_ERROR "32-bit builds are not supported. Please use a 64-bit generator/toolchain.") | ||
| endif() |
There was a problem hiding this comment.
The configure.ac check is not very useful, it's meant to enforce 32 bit builds when you don't want 64 bit ones, on 64 bit OSes/compilers. It can be safely removed, without replacing it with anything. People that want 32 bit builds, will provide the compiler flags themselves, or use a 32 bit cross-compiler
| capiif.h | ||
| ) | ||
|
|
||
| list(APPEND CHAR_HDRS ${CONFIG_HDRS}) |
There was a problem hiding this comment.
These should probably be included indirectly by depending on common or a dedicated library
| lapiif.h | ||
| ) | ||
|
|
||
| list(APPEND LOGIN_HDRS ${CONFIG_HDRS}) |
There was a problem hiding this comment.
Same as above, these should just be a dependency on common or dedicated header-only library
| vending.h | ||
| ) | ||
|
|
||
| list(APPEND MAP_HDRS ${CONFIG_HDRS}) |
| message(FATAL_ERROR "In-source builds are not allowed. Please create a separate build directory.") | ||
| endif() | ||
|
|
||
| if(NOT CMAKE_INSTALL_PREFIX) |
There was a problem hiding this comment.
This is always set by cmake if not passed so the check will be always false. See https://cmake.org/cmake/help/v4.3/variable/CMAKE_INSTALL_PREFIX.html and https://cmake.org/cmake/help/v4.3/variable/CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT.html#variable:CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT
|
|
||
| # Appends a list of headers to global property ALL_HDRS with their absloute path | ||
| # _headers list of headers | ||
| function(add_header_all_list _headers) |
There was a problem hiding this comment.
What do we need this for? Can we avoid messing with lists of headers like this?
|
|
||
| # Appends a list of headers to global property ALL_CONFIG_HDRS with their absloute path | ||
| # _headers list of headers | ||
| function(add_header_all_config_list _headers) |
There was a problem hiding this comment.
It would be nice not to have to mess with lists of files like this. What do we need this for, and can we get rid of it?
| # List of include files | ||
| set(HPMHOOKING_INCLUDES "") | ||
|
|
||
| file(GLOB include_files RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} *.inc) |
There was a problem hiding this comment.
Might be better to remove the glob since we know the names we expect
| // check is TESTVAR defined from custom compile flags | ||
| #ifndef TESTVAR | ||
| #error TESTVAR not defined | ||
| #endif | ||
|
|
||
| // check is TESTVAR2 defined to value 2 from custom compile flags | ||
| #if !(TESTVAR2 == 2) | ||
| #error TESTVAR2 not defined | ||
| #endif |
There was a problem hiding this comment.
I'd keep these checks to demonstrate/test that the flags are added
|
|
||
| /// Sample Hercules Plugin | ||
|
|
||
| #include "plugins/sample/sample.h" |
There was a problem hiding this comment.
If this requires a full path, then we're doing something wrong in the cmake. We should ensure that each plugin has its own folder in its include path
There was a problem hiding this comment.
oh it doesn't require that that's a little screw up.
| // check is TESTVAR defined from custom compile flags | ||
| #ifndef TESTVAR | ||
| #error TESTVAR not defined | ||
| #endif | ||
|
|
||
| // check is TESTVAR2 defined to value 2 from custom compile flags | ||
| #if !(TESTVAR2 == 2) | ||
| #error TESTVAR2 not defined | ||
| #endif |
There was a problem hiding this comment.
I don't think this needs to be in a header, it can be in the plugin's main .c file in my opinion (it's not defining anything that it needs to share)
If you want to show an example of a plugin that has its own .h file, then maybe add something defined or declared here that is used in the .c file
| echo | ||
| "Preprocessing files" | ||
| COMMAND | ||
| ./preprocess.py |
There was a problem hiding this comment.
This kind of invocation is a bit specific to unix systems, so the whole thing should probably go behind a check to prevent it from choking on windows.
Alternatively (and probably better), this could call python explicitly, like it does for perl below. That way it should probably run on any OS.
Also, since it checks for perl's presence it should also check for python
| ${HERC_LIBCONF_HDRS} | ||
| ) | ||
|
|
||
| target_compile_options(hercules-libconfig PRIVATE -Wno-null-dereference) |
There was a problem hiding this comment.
gcc/clang-specific option, will require a conditional
MishimaHaruna
force-pushed
the
cmake
branch
5 times, most recently
from
fe1bc9f to
38981ef
Compare
…n empty `./configure` run Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…ng on our own Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
… 3rdparty source folder Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…hrough conan Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com> Signed-off-by: Haru <haru@dotalux.com>
…t being empty Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…n CMakeFileLists.txt file This would allow for more complex multi-file plugins and better customized build rules per plugin. Due to this change any plugin that includes HPMHooks using path `plugins/HPMHooking.h` will now have to include it as `plugins/HPMHooking/HPMHooking.h` Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…ile tends to have possible false positives Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…d unnecessirly Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…nflict with windows setup and corrected the RPATH for plugins to reflect directory structure Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…oesn't depend on libbacktrace now the core will save current executable path to core->executable_path Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…d change its name to herc-plugins Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
…changes for a 64bit build Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
… sanitizer build as conan doesn't support preview releases Signed-off-by: Ibrahim Zidan <brahem@aotsw.com>
| # include <unistd.h> | ||
| #else | ||
| #endif | ||
| #if defined(WIN32) |
There was a problem hiding this comment.
testing for WIN32 right after testing for _WIN32 will look odd to those in the future who will git blame this and end up finding this commit.
I would instead group all the unix checks within the else branch:
#if defined(WIN32)
#include "common/winapi.h"
#else
#include <unistd.h>
#if defined(__sun)
//...
#elif // ...
#endif
#endif
| #include <stdlib.h> // atoi | ||
| #ifdef WIN32 | ||
| # include <windows.h> | ||
| # include <winternl.h> |
There was a problem hiding this comment.
What's this header providing here? It might be worth adding a comment to it since it's a quite unusual (and not very recommended) header to include
| option(ENABLE_CLASSIC_AUTOSPELL "Enables classic auto spell list" OFF) | ||
|
|
||
| if(ENABLE_TESTING) | ||
| enable_testing() |
There was a problem hiding this comment.
This should probably not be part of this commit since it's not mentioned in the commit message.
It's also called twice (the second time in the src/test CMakeLists)
| # run_test spinlock # Not running the spinlock test for the time being (too time consuming) | ||
| # run_test libconfig | ||
| # run_test chunked | ||
| run_test spinlock |
There was a problem hiding this comment.
Was this uncommented accidentally?
| run_server ./map-server "$PLUGINS --load-plugin db2sql --mobdb2sql" | ||
| run_server ./bin/map-server "$PLUGINS --load-plugin db2sql --db2sql" | ||
| run_server ./bin/map-server "$PLUGINS --load-plugin db2sql --itemdb2sql" | ||
| run_server ./bin/map-server "$PLUGINS --load-plugin db2sql --mobdb2sql" | ||
| # look like works on windows only | ||
| # echo "run all servers with dbghelpplug" | ||
| # run_server ./login-server "$PLUGINS --load-plugin dbghelpplug" | ||
| # run_server ./char-server "$PLUGINS --load-plugin dbghelpplug" | ||
| # run_server ./map-server "$PLUGINS --load-plugin dbghelpplug" |
There was a problem hiding this comment.
These lines should be removed along with the dbghelpplug plugin
| !/src/plugins/dbghelpplug/ | ||
| !/src/plugins/dbghelpplug/* |
There was a problem hiding this comment.
These lines should be removed along with the dbghelpplug plugin
| @@ -0,0 +1,570 @@ | |||
| # This file is part of Hercules. | |||
There was a problem hiding this comment.
This script requires that the user passes some arguments (in particular the conan ones), that might be hard to remember. They should be documented in the project readme, and perhaps it would be nice to provide a convenience script (.sh and .bat/.ps1) with reasonable defaults
| add_compile_definitions(WIN32 _WIN32 __WIN32 _CRT_SECURE_NO_DEPRECATE _CRT_NONSTDC_NO_DEPRECATE FD_SETSIZE=4096 WIN32_LEAN_AND_MEAN) | ||
| # Define arch | ||
| if(CMAKE_SIZEOF_VOID_P EQUAL 4) | ||
| add_compile_definitions(_X86_) |
There was a problem hiding this comment.
I don't see this macro used anywhere in the code. Is it necessary?
| endif() | ||
|
|
||
| if(ENABLE_WERROR) | ||
| testadd_warning_compiler_flag(error REQUIRED) |
There was a problem hiding this comment.
This flag is gcc/clang-specific. The MSVC equivalent is /WX.
Perhaps this option could be renamed to WARNINGS_ARE_ERRORS and made it work across compilers instead?
| endif() | ||
|
|
||
| if(ENABLE_LTO) | ||
| testadd_compiler_flag(flto REQUIRED) |
There was a problem hiding this comment.
gcc/clang-specific flags - this will fail with unexplanatory error messages on MSVC, so it could perhaps go into an if else with a proper message(FATAL_ERROR
| endif() | ||
|
|
||
| if(ENABLE_SANITIZE) | ||
| add_compile_definitions(SANITIZE) |
There was a problem hiding this comment.
gcc/clang-specific flags - this will silently do nothing on MSVC, so it could perhaps go into an if else with a proper message(FATAL_ERROR) so that the user doesn't have any false expectations
3 participants
Pull Request Prelude
Changes Proposed
This Pull request changes our compilation generation tool to cmake so unify all platforms under a single umberalla, we also added conan as our dependency manager to get rid of as much of 3rdparty as possible and also the DLLs deployed for windows.
Issues addressed: