CMake: Toolchain fixes #631
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| Name: libjpeg | ||
| Description: A SIMD-accelerated JPEG codec that provides the libjpeg API | ||
| Version: 2.0.4 | ||
| Libs: -l${NXDK_DIR}/lib/libjpeg.lib | ||
| Cflags: -I${NXDK_DIR}/lib/libjpeg/libjpeg-turbo -I${NXDK_DIR}/lib/libjpeg |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # FPHSA_NAME_MISMATCHED is require to suppress warning message | ||
| set(FPHSA_NAME_MISMATCHED TRUE) | ||
| include(FindPkgConfig) | ||
| pkg_check_modules(sdl2 REQUIRED sdl2) | ||
| unset(FPHSA_NAME_MISMATCHED) | ||
|
|
||
| add_library(SDL2::SDL2 INTERFACE IMPORTED) | ||
| set(SDL2_INCLUDE_DIRS ${sdl2_INCLUDE_DIRS}) | ||
| set(SDL2_LIBRARIES ${sdl2_LIBRARIES}) | ||
| set(SDL2_LINK_LIBRARIES ${sdl2_LINK_LIBRARIES}) | ||
| set_target_properties(SDL2::SDL2 PROPERTIES | ||
| INTERFACE_INCLUDE_DIRECTORIES "${SDL2_INCLUDE_DIRS}" | ||
| INTERFACE_LINK_LIBRARIES "${SDL2_LINK_LIBRARIES}" | ||
| # NOTE: pkg_check_modules' Cflags definition for "XBOX" did not get included and was passed to CFLAGS_OTHER for some reason... | ||
|
There was a problem hiding this comment. nit: Most comments in this PR have typos ( There was a problem hiding this comment. It is not a typo, you can find it at https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_check_modules . Since the function's name is already plural, then there shouldn't be an s after apostrophe? There was a problem hiding this comment. In this case it's actually fine, but other comments have spelling/grammar issues. |
||
| INTERFACE_COMPILE_OPTIONS "${sdl2_CFLAGS}" | ||
| ) | ||
| set(SDL2_FOUND 1) | ||
| add_library(SDL2 ALIAS SDL2::SDL2) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| # NOTE: custom FindThreads module is a requirement due to CMake's FindThreads module is looking for pThreads. | ||
| # nxdk's toolchain is using system named Generic instead of "Windows" which could had work but inaccurate name. | ||
| # xboxkrnl and pdclib have C11 thread support out of the box | ||
| set(CMAKE_HAVE_THREADS_LIBRARY 1) | ||
| set(Threads_FOUND TRUE) | ||
| add_library(Threads::Threads INTERFACE IMPORTED) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,10 @@ | ||
| # NOTE: Require to prevent obtaining pre-existing information | ||
| # and calling macro repeatly unintentionally. | ||
| if (__TOOLCHAIN_NXDK) | ||
| return() | ||
| endif() | ||
| set(__TOOLCHAIN_NXDK 1) | ||
|
|
||
| if(DEFINED ENV{NXDK_DIR}) | ||
| set(NXDK_DIR $ENV{NXDK_DIR}) | ||
| else() | ||
|
|
@@ -19,7 +26,7 @@ set(WIN32 1) | |
| set(NXDK 1) | ||
|
|
||
| set(CMAKE_C_COMPILER "${NXDK_DIR}/bin/${TOOLCHAIN_PREFIX}-cc") | ||
| set(CMAKE_C_STANDARD_LIBRARIES "${NXDK_DIR}/lib/libwinapi.lib ${NXDK_DIR}/lib/xboxkrnl/libxboxkrnl.lib ${NXDK_DIR}/lib/libxboxrt.lib ${NXDK_DIR}/lib/libpdclib.lib ${NXDK_DIR}/lib/libnxdk_hal.lib ${NXDK_DIR}/lib/libnxdk.lib ${NXDK_DIR}/lib/nxdk_usb.lib ${NXDK_DIR}/lib/libnxdk_net.lib") #"${CMAKE_CXX_STANDARD_LIBRARIES_INIT}" | ||
|
There was a problem hiding this comment. I'm not sure why networking would be part of the C standard. Also see https://github.com/JayFoxRox/nxdk/pull/84/files#r611210118 Once nxdk has winsock, that could also take care of it. There was a problem hiding this comment. NXDK_NET option is dropped, so it is always included. See #575 pull request changes. There was a problem hiding this comment. There isn't a good reason why it should always be linked (for CMake and the legacy nxdk "build-application"-makefile alike). But good reasons to always build it (as part of the nxdk "build-toolchain"-makefile). I can only speculate, but I assume the intention in #575 was to always build the lib, so the nxdk makefile is used for actually building nxdk (= libraries usable by pkg-config / CMake / ..), instead of building a single application (= decision which libraries to include); the PR description says:
Maybe @thrimbor can clarify? I believe, in your case, the CMake user should now choose which libraries to include (but CMake will take care of the standard library, hence you need to specify those and dependencies for the C/C++ standard). There was a problem hiding this comment. One small thing I would like to add into JayFoxRox's comment. If download source code repo without build nxdk via make. Then use CMake, the toolchain, to build user's application will not setup the necessary build files due to nxdk did not get built. I took a punch in my own eye not realizing this and didn't think about build nxdk via make. Mainly because I thought it was already handled through nxdk's makefile from the toolchain script. I was completely wrong. There was a problem hiding this comment. The changes in #575 regarding building were mostly to simplify building nxdk itself - adjusting compiler flags and include directories based on make vars is a hot mess when building the libraries. Regarding the change above, I think I'm ok with it for now. Ultimately I'd like to see a winsock library (next lwIP release should hopefully bring us closer to being able to implement this), and then we can have a In the long term I'd like to see the low-level networking and USB libs removed from the standard libs and have them explicitly linked against if the app touches low-level stuff directly, or automatically pulled in by higher level libraries that are explicitly specified by the user such as SDL or winsock. But as long as we can't have that, imho linking them in might be the lesser of two evils. |
||
| set(CMAKE_C_LINK_EXECUTABLE "${NXDK_DIR}/bin/${TOOLCHAIN_PREFIX}-link <FLAGS> <CMAKE_C_LINK_FLAGS> <LINK_FLAGS> <OBJECTS> -out:<TARGET> <LINK_LIBRARIES>") | ||
|
|
||
| set(CMAKE_CXX_COMPILER "${NXDK_DIR}/bin/${TOOLCHAIN_PREFIX}-cxx") | ||
|
|
@@ -45,4 +52,34 @@ set(_CMAKE_C_IPO_SUPPORTED_BY_CMAKE YES) | |
| set(_CMAKE_C_IPO_MAY_BE_SUPPORTED_BY_COMPILER YES) | ||
| set(CMAKE_C_COMPILE_OPTIONS_IPO -flto) | ||
|
|
||
| # TODO: CMAKE_MODULE_PATH is intended for project level than toolchain, | ||
| # find out what's wrong with CMAKE_FIND_ROOT_PATH as it is not working for some reason? | ||
| set(CMAKE_MODULE_PATH "${NXDK_DIR}/share/cmake/Modules") | ||
| set(PKG_CONFIG_EXECUTABLE "${NXDK_DIR}/bin/nxdk-pkg-config" CACHE STRING "Path to pkg-config") | ||
|
|
||
| # Additional toolchain steps required for create xbe and xiso files. | ||
| macro(add_executable) | ||
| # Perform normal call since there's no modification needed except for additional steps require | ||
| _add_executable(${ARGV}) | ||
|
|
||
| # If author request generate iso file, then prepare the command here. | ||
| if(GEN_XISO) | ||
| set(XISO_OUTPUT "${NXDK_DIR}/tools/extract-xiso/build/extract-xiso" -c "$<TARGET_FILE_DIR:${ARGV0}>/bin" "$<TARGET_FILE_DIR:${ARGV0}>/${GEN_XISO}") | ||
| endif() | ||
|
There was a problem hiding this comment. I suggest to leave XISO out of this. It also doesn't feel CMake-ish. It also doesn't handle a common use-case where a user will want to selectively add more files to the XISO; so this will need a better design. Also, if we support In my own CMake work I just created another CMake function but I believe I still did XISO creation manually in a shell script for flexibility. There was a problem hiding this comment. I like the concept of making it as CMake function which I now notice from your link. Initially I'm following how makefile handling things which currently use There was a problem hiding this comment.
I don't think so. Users who move from the makefile to CMake will have to change their code anyway, so they can add the function call themselves. I believe the original nxdk makefile (for use outside of nxdk internally) will also be retired eventually, and users who still want to use a makefile can use their own OR find some build-system they like (which doesn't have to be CMake). There was a problem hiding this comment. Is there opened ticket discussed about it and/or have list of tasks for moving away from original nxdk makefile to have it done through toolchain script? I am currently unable to find the information in the repository. If there is or isn't, it will be helpful to follow through guideline to make a working toolchain as possible and have tasks done. There was a problem hiding this comment.
In summary, it's better off to perform xiso creation from application preferred target's POST_BUILD process with CMake's add_custom_command function; via nxdk wiki CMake's getting started/howto. |
||
|
|
||
| # If custom title is not given, use given executable name. | ||
| if(NOT XBE_TITLE) | ||
|
There was a problem hiding this comment. I believe this variable name needs a prefix to not clash with user variables in CMake? There's probably some convention for this in CMake. There was a problem hiding this comment. It is only triggered when user call add_executable function. Though, I will check with forked NevolutionX repo with fixed CMake support to be sure it doesn't return back to user's CMakeList. There was a problem hiding this comment. I can confirm XBE_TITLE is stored back into user's CMakeList, will need to make a fix for this. concept fix: if(NOT XBE_TITLE)
set(<prefix>_XBE_TITLE ${ARGV0})
else()
set(<prefix>_XBE_TITLE ${XBE_TITLE})
endif()As for prefix, not sure if we should use NXDK or CMAKE prefix. There was a problem hiding this comment. I'd go for There was a problem hiding this comment.
That's the plan. I must had forgot to mentioned that in my previous comment. |
||
| set(XBE_TITLE ${ARGV0}) | ||
| endif() | ||
|
|
||
| # Attach additional toolchain steps to executable project. | ||
| add_custom_command(TARGET ${ARGV0} POST_BUILD | ||
| COMMAND mkdir -p $<TARGET_FILE_DIR:${ARGV0}>/bin | ||
| # Generate xbe binary conversion | ||
| COMMAND "${NXDK_DIR}/tools/cxbe/cxbe" "-OUT:$<TARGET_FILE_DIR:${ARGV0}>/bin/default.xbe" "-TITLE:${XBE_TITLE}" "$<TARGET_FILE_DIR:${ARGV0}>/${ARGV0}.exe" | ||
| # Optionally generate xiso file if requested | ||
| COMMAND "$<$<BOOL:${XISO_OUTPUT}>:$<JOIN:${XISO_OUTPUT},;>>" | ||
| COMMAND_EXPAND_LISTS | ||
| VERBATIM | ||
| ) | ||
| endmacro(add_executable) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need all of these? I don't expect regular application code to have to deal with nvnetdrv for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the option of NXDK_NET was dropped in main makefile, These includes came from these lines.
nxdk/lib/net/Makefile
Lines 22 to 27 in da6a0bd
Do I know which includes folder actually used for application? No, I didn't have time to check all other applications using nxdk. Since I was too busy figuring out what are the major problems using CMake in codespace environment. I can recheck it later for what's really unnecessary for user-level application development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to check all applications. You seem to be testing with NevolutionX, which includes network support, so you can check whether it builds without the
nvnetdrvandnforceifdirectories, and if it does, you can remove them. I assume only the lwip directory is necessary because that supplies the actual socket layer for the application, the other two should only be necessary for building nxdk itself.