CMake: Toolchain fixes

[RadWolfie]

After some troubleshooting process to figure out what went wrong with docker image for cmake usage. I found out there are several both manually incorrect coded and missing some support.

• pkgconfig for libjpeg missed replacement of round to parenthesis brackets
• Whole *.pc files were missing -l, lowercase L, before each library files in Libs: variable. I had confirmed the correction now appear in <PREFIX>_LIBRARIES and <PREFIX>_LINK_LIBRARIES from output message test in cmake.
• FindThreads.cmake module is a requirement due to stock module will try to find pthread instead and fail. Since CMAKE_SYSTEM_NAME variable is set to Generic instead of Windows which fall back to unix-like's search method.
• FindSDL2.cmake should be a requirement anyway due to definitions does not get include and only be found in <PREFIX>_CFLAGS_OTHER. Although, it does show up in <PREFIX>_CFLAGS except got exclude at compile/link time for unknown reason.
• For unknown reason, find_package did not look in nxdk's directory therefore require manually add path into CMAKE_MODULE_PATH.
  NOTE: CMAKE_MODULE_PATH is document intended for project level than toolchain.
• Add xbe conversion and check if author want xiso generated after compilation completion.

The changes made were tested with experimental package published on GitHub to test with NevolutionX application using cmake. It is able to compile with nxdk and with linux libraries (require install sdl2 packages).

[JayFoxRox]

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 GEN_XISO now, then we can't remove it without breaking projects.
As nxdk matures, we should avoid breakage like that.

---

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.

[RadWolfie]

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 GEN_XISO. If preferred the change to CMake function, I'll be happily change it. Yet may need to preserve backward compatibility for the change from make to CMake?

[JayFoxRox]

Yet may need to preserve backward compatibility for the change from make to CMake?

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).
Also see my work on autoconf etc. - while CMake support is part of nxdk, it's trivial to use other build-systems after the existing refactors to the build-system (which only added CMake, for better or worse)

[RadWolfie]

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.

[RadWolfie]

Xiso creation is one of those - it gives very little control over the end result (on-disc layout, non-1:1 directory mapping etc.), and technically isn't required to get something running on the Xbox, so imho we can leave that out the CMake toolchain file. It's still frequently used for quick emulator testing, so my suggestion would be to have a wiki entry "Creating an xiso from CMake" or something that supplies code that can just be dropped in.

Quote from thrimbor

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.

[JayFoxRox]

I'm not sure why networking would be part of the C standard.
Applications which want to use this, should add it explicitly.

Also see https://github.com/JayFoxRox/nxdk/pull/84/files#r611210118

Once nxdk has winsock, that could also take care of it.

[RadWolfie]

NXDK_NET option is dropped, so it is always included. See #575 pull request changes.

[JayFoxRox]

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:

To make building the libraries easier, the network code is now always built.

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).
The fewer symbols our CMake brings, the better (smaller binaries, faster linking, less symbol name collisions, easier to customize your drivers/backends).

[RadWolfie]

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.

[thrimbor]

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 ws2_32.lib that I'd expect the user to have to explicitly specify, which can then pull in the other networking libraries via #pragma comment(lib,"libnxdk_net.lib") (this is done a lot on Windows). Atm our messy buildsystem relies on dependencies being explicit, so we can't really utilize this as long as we have a makefile that's expected to handle both building nxdk itself and applications.

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.

[JayFoxRox]

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.

[RadWolfie]

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. It may also apply to GEN_XISO variable too.

[RadWolfie]

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.

[thrimbor]

I'd go for _NXDK_XBE_TITLE, the leading underscore seems to be convention in CMake for variables that are considered internal to a module.
Could we also do an unset when we're done with it?

[RadWolfie]

Could we also do an unset when we're done with it?

That's the plan. I must had forgot to mentioned that in my previous comment.

[JayFoxRox]

nit: Most comments in this PR have typos (pkg_check_modules') or poor wording. Someone should spell check these.

[RadWolfie]

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?

[thrimbor]

In this case it's actually fine, but other comments have spelling/grammar issues.

[RadWolfie]

@thrimbor, can we get a public guideline what toolchain should do? There's no information about it. Such as GEN_XISO should be replace as a function to permit title developers to create their own xiso image. Using reference of makefile, both main and samples, files from nxdk here isn't helpful. Plus there are some unresolved discussion that are welcome further discussions. 🙏

[thrimbor]

The closest thing to a guideline I have is "don't rely on anything the makefile does for you". It didn't really start out as the base for a toolchain, the goal probably was to have something that makes it as simple as possible to build the example it came with. As such it brings a few convenience functions, but with limitations and and reliance on specifics of flawed tools.

Xiso creation is one of those - it gives very little control over the end result (on-disc layout, non-1:1 directory mapping etc.), and technically isn't required to get something running on the Xbox, so imho we can leave that out the CMake toolchain file. It's still frequently used for quick emulator testing, so my suggestion would be to have a wiki entry "Creating an xiso from CMake" or something that supplies code that can just be dropped in.

Feature parity between our current makefile and CMake to ease the switch is not a big concern to me - getting rid of the way we currently build things will break applications relying on it and require major adjustments anyway.

[thrimbor]

I'm not close to having reviewed the whole PR yet (my free time is rather fragmented), but regarding 702b32e:

Is that change required by CMake? As it is now that commit breaks direct usage of nxdk-pkg-config as lld-link does not understand -l parameters (it takes link.exe syntax as opposed to ld)

[RadWolfie]

When I was debugging the issues for CMake problems from codespace I'll quote what I had found from OP post:

• Whole *.pc files were missing -l, lowercase L, before each library files in Libs: variable. I had confirmed the correction now appear in <PREFIX>_LIBRARIES and <PREFIX>_LINK_LIBRARIES from output message test in cmake.

Libs: accept -L<path to lib dir> and -l<path to lib file> according to pkg-config documentation I had looked at. If that's the wrong source to look at the documentation, please direct me to the right source. Otherwise, without -l argument included as I quoted will not work correctly with CMake.

[thrimbor]

Like that guide says it's for Unix-like systems. -L and -l are flags for ld and not compatible with link.exe so we cannot use them.

Otherwise, without -l argument included as I quoted will not work correctly with CMake.

What will not work? What was the issue and how can I reproduce it?

/edit:
There seems to be a flag that's not documented in the man page, --msvc-syntax. That might be the way to go, depending on whether this will work with other buildsystems such as meson.

[RadWolfie]

What will not work? What was the issue and how can I reproduce it?

You can use https://github.com/RadWolfie/NevolutionX/tree/dev-container branch, select green code button dropdown menu > three dots (...) > + New with options... > it will open a page with options. The only thing need to change is Dev container configuration to "Xbox Development". Once done, you can press "Create codespace". From there, you can test compile build out of the box which is currently using my own nxdk package. The modified I made to CMakeLists.txt file to show the output are:

# Place this after "pkg_search_module(SDL2TTF REQUIRED SDL2_ttf)" line.
message("SDL2TTF libraries: ${SDL2TTF_LIBRARIES}")
message("SDL2TTF link libraries: ${SDL2TTF_LINK_LIBRARIES}")
message("SDL2TTF cflags: ${SDL2TTF_CFLAGS}")

which output:

[cmake] SDL2TTF libraries: /usr/src/nxdk/lib/libSDL_ttf.lib;/usr/src/nxdk/lib/libfreetype.lib;/usr/src/nxdk/lib/libSDL2.lib
[cmake] SDL2TTF link libraries: /usr/src/nxdk/lib/libSDL_ttf.lib;/usr/src/nxdk/lib/libfreetype.lib;/usr/src/nxdk/lib/libSDL2.lib
[cmake] SDL2TTF cflags: -I/usr/src/nxdk/lib/sdl;-I/usr/src/nxdk/lib/sdl/SDL_ttf;-I/usr/src/nxdk/lib/sdl/SDL2/include;-DXBOX

To reproduce the issue from upstream. You will need go into .devcontainer/xbox/Dockerfile file and change very first line to FROM ghcr.io/xboxdev/nxdk:latest. After the change click "Codespaces" from bottom left corner, select "Full rebuild container". Then you will want to comment out find_package(Threads REQUIRED) and find_package(SDL2 REQUIRED) lines. As they will block continue run generate build setup.
After that and do the build, the output will be:

[cmake] SDL2TTF libraries: 
[cmake] SDL2TTF link libraries: 
[cmake] SDL2TTF cflags: -I/usr/src/nxdk/lib/sdl;-I/usr/src/nxdk/lib/sdl/SDL_ttf;-I/usr/src/nxdk/lib/sdl/SDL2/include;-DXBOX

As for cannot have -L / -l is a bit confusing... I ran a CI test for pkg-config command on all three available platforms from GitHub action: https://github.com/RadWolfie/cross-platform-pkg-config-test/actions/runs/5634597949
On Windows, it even says -lcrypto, -lpng16 -lz, and -ljpeg via doing the commands below:

pkg-config libcrypto --libs
pkg-config libpng --libs
pkg-config libjpeg --libs

🤔
After your edited comment, I append another job test except all has failed for --msvc-syntax. I have not used pkg-config on Windows platform.

on side note: I didn't know it is possible to compile nxdk with msvc?

[thrimbor]

[cmake] SDL2TTF libraries: 
[cmake] SDL2TTF link libraries: 
[cmake] SDL2TTF cflags: -I/usr/src/nxdk/lib/sdl;-I/usr/src/nxdk/lib/sdl/SDL_ttf;-I/usr/src/nxdk/lib/sdl/SDL2/include;-DXBOX

I see. Annoying that CMake seems to mess with these files directly, but I guess we'll need to keep CMake happy.

As for cannot have -L / -l is a bit confusing... I ran a CI test for pkg-config command on all three available platforms from GitHub action: https://github.com/RadWolfie/cross-platform-pkg-config-test/actions/runs/5634597949 On Windows, it even says -lcrypto, -lpng16 -lz, and -ljpeg via doing the commands below:

That's expected, pkg-config defaults to assuming a Unix-like environment (mysys/Cygwin on Windows).

thinking After your edited comment, I append another job test except all has failed for --msvc-syntax. I have not used pkg-config on Windows platform.

That's annoying - there are three different implementations of pkg-config:

• pkg-config: The original implementation that supports --msvc-syntax
• pkgconf: Fully compatible reimplementation that seems to get preferred by Linux distros nowadays
• PkgConfig: A reimplementation in Perl that is not fully compatible and does not support --msvc-syntax

For some reason you seem to get PkgConfig in your tests, which sucks. On my system (Arch Linux), I get pkgconf (version 1.8.1), which works just fine:

If you want to experiment you could try calling pkgconf instead of pkg-config, maybe it's installed but not the default. In the worst case we'll have to ship pkgconf with nxdk.

on side note: I didn't know it is possible to compile nxdk with msvc?

It's not, but the tools we use (lld-link) are designed as drop-in replacements for MSVC tools and follow the same cli syntax. We also use the same ABI so in theory you should be able to mix and match LLVM and MSVC when building something that uses nxdk.

[RadWolfie]

From codespace using the docker image, it is using pkgconf (version 1.9.4) when I call pkg-config -h and pkg-config --version. To be sure it's not a fluke, I can confirm it is the same via call pkgconf. I can also confirm --msvc-syntax does appear from -h argument. I took a look in CMakeCache.txt file which shows FIND_PACKAGE_MESSAGE_DETAILS_PkgConfig:INTERNAL=[/usr/src/nxdk/bin/nxdk-pkg-config][v1.9.4()] meaning it does use pkgconf.

I tried downgrade pkgconf to 1.8.1 yet still it gives me blank library inputs.

message("nxdk-pkg-config version:")
execute_process(COMMAND nxdk-pkg-config --version
)
message("nxdk-pkg-config SDL2_image --libs outputs:")
execute_process(COMMAND nxdk-pkg-config SDL2_image --libs
)
message("nxdk-pkg-config SDL2_image --libs --msvc-syntax outputs:")
execute_process(COMMAND nxdk-pkg-config SDL2_image --libs --msvc-syntax
)

message("using CMake's PkgConfig module: pkg_search_module")
pkg_search_module(SDL2IMAGE REQUIRED SDL2_image)
message("SDL2IMAGE libraries: ${SDL2IMAGE_LIBRARIES}")
message("SDL2IMAGE link libraries: ${SDL2IMAGE_LINK_LIBRARIES}")
message("SDL2IMAGE cflags: ${SDL2IMAGE_CFLAGS}")

The output will be:

[cmake] nxdk-pkg-config version:
[cmake] 1.8.1
[cmake] nxdk-pkg-config SDL2_image --libs outputs:
[cmake] /usr/src/nxdk/lib/libSDL2_image.lib /usr/src/nxdk/lib/libSDL2.lib /usr/src/nxdk/lib/libjpeg.lib /usr/src/nxdk/lib/libpng.lib /usr/src/nxdk/lib/libzlib.lib 
[cmake] nxdk-pkg-config SDL2_image --libs --msvc-syntax outputs:
[cmake] 
[cmake] using CMake's PkgConfig module: pkg_search_module
[cmake] SDL2IMAGE libraries: 
[cmake] SDL2IMAGE link libraries: 
[cmake] SDL2IMAGE cflags: -I/usr/src/nxdk/lib/sdl/SDL2_image;-I/usr/src/nxdk/lib/sdl/SDL2/include;-DXBOX;-I/usr/src/nxdk/lib/libjpeg/libjpeg-turbo;-I$(NXDK_DIR)/lib/libjpeg;-I/usr/src/nxdk/lib/libpng;-I/usr/src/nxdk/lib/libpng/libpng;-I/usr/src/nxdk/lib/zlib/zlib;-DZ_SOLO

If I change SDL2_image.pc to include `-l` before library file. Then it outputs:

[cmake] nxdk-pkg-config version:
[cmake] 1.8.1
[cmake] nxdk-pkg-config SDL2_image --libs outputs:
[cmake] -l/usr/src/nxdk/lib/libSDL2_image.lib /usr/src/nxdk/lib/libSDL2.lib /usr/src/nxdk/lib/libjpeg.lib /usr/src/nxdk/lib/libpng.lib /usr/src/nxdk/lib/libzlib.lib 
[cmake] nxdk-pkg-config SDL2_image --libs --msvc-syntax outputs:
[cmake] /usr/src/nxdk/lib/libSDL2_image.lib.lib 
[cmake] -- Checking for one of the modules 'SDL2_image'
[cmake] using CMake's PkgConfig module: pkg_search_module
[cmake] SDL2IMAGE libraries: /usr/src/nxdk/lib/libSDL2_image.lib
[cmake] SDL2IMAGE link libraries: /usr/src/nxdk/lib/libSDL2_image.lib
[cmake] SDL2IMAGE cflags: -I/usr/src/nxdk/lib/sdl/SDL2_image;-I/usr/src/nxdk/lib/sdl/SDL2/include;-DXBOX;-I/usr/src/nxdk/lib/libjpeg/libjpeg-turbo;-I$(NXDK_DIR)/lib/libjpeg;-I/usr/src/nxdk/lib/libpng;-I/usr/src/nxdk/lib/libpng/libpng;-I/usr/src/nxdk/lib/zlib/zlib;-DZ_SOLO

So, in summary if not include `-l` before library files will output SDL2_image except not the whole library dependencies.

With the correction from PR outputs:

[cmake] nxdk-pkg-config version:
[cmake] 1.8.1
[cmake] nxdk-pkg-config SDL2_image --libs outputs:
[cmake] -l/usr/src/nxdk/lib/libSDL2_image.lib -l/usr/src/nxdk/lib/libSDL2.lib -l/usr/src/nxdk/lib/libjpeg.lib -l/usr/src/nxdk/lib/libpng.lib -l/usr/src/nxdk/lib/libzlib.lib
[cmake] nxdk-pkg-config SDL2_image --libs --msvc-syntax outputs:
[cmake] /usr/src/nxdk/lib/libSDL2_image.lib.lib /usr/src/nxdk/lib/libSDL2.lib.lib /usr/src/nxdk/lib/libjpeg.lib.lib /usr/src/nxdk/lib/libpng.lib.lib /usr/src/nxdk/lib/libzlib.lib.lib 
[cmake] using CMake's PkgConfig module: pkg_search_module
[cmake] SDL2IMAGE libraries: /usr/src/nxdk/lib/libSDL2_image.lib;/usr/src/nxdk/lib/libSDL2.lib;/usr/src/nxdk/lib/libjpeg.lib;/usr/src/nxdk/lib/libpng.lib;/usr/src/nxdk/lib/libzlib.lib
[cmake] SDL2IMAGE link libraries: /usr/src/nxdk/lib/libSDL2_image.lib;/usr/src/nxdk/lib/libSDL2.lib;/usr/src/nxdk/lib/libjpeg.lib;/usr/src/nxdk/lib/libpng.lib;/usr/src/nxdk/lib/libzlib.lib
[cmake] SDL2IMAGE cflags: -I/usr/src/nxdk/lib/sdl/SDL2_image;-I/usr/src/nxdk/lib/sdl/SDL2/include;-DXBOX;-I/usr/src/nxdk/lib/libjpeg/libjpeg-turbo;-I/usr/src/nxdk/lib/libjpeg;-I/usr/src/nxdk/lib/libpng;-I/usr/src/nxdk/lib/libpng/libpng;-I/usr/src/nxdk/lib/zlib/zlib;-DZ_SOLO

TLDR version: Alpine docker's pkgconf didn't output the libraries with --msvc-syntax. With the correction from this pull request, it does. 🤷 I think it's better to communicate with pkgconf developers for better understanding the specification of *.pc files for libs: field.

[thrimbor]

TLDR version: Alpine docker's pkgconf didn't output the libraries with --msvc-syntax. With the correction from this pull request, it does. 🤷 I think it's better to communicate with pkgconf developers for better understanding the specification of *.pc files for libs: field.

There seems to be a misunderstanding here, let me clarify what I meant.

The way we currently have things set up makes pkg-config emit link.exe-style parameters which is fine for using them directly or with meson, but doesn't work with CMake because it expects to see ld-style parameters.
With your changes CMake is happy because it now gets ld-style parameters, but direct usage doesn't work (and I assume meson doesn't either).
With --msvc-syntax, we can keep your changes and still get link.exe-style parameters because pkg-config will convert them for us when passing that. For direct usage the user will have to specify that parameter, for meson we can probably add it to the toolchain file.

The only requirement is that we have a compatible implementation of pkg-config. On our docker image that's not a problem because we control what software it contains, and I don't worry about Linux distros either.
GH's runners may be problematic though because they ship with that shitty Perl version.

[thrimbor]

I've gone through the code, and as far as I can see apart from the bits I'm mentioning here all issues have already been raised by jfr.

[thrimbor]

Do we really need all of these? I don't expect regular application code to have to deal with nvnetdrv for example.

[RadWolfie]

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

	NXDK_CFLAGS += -I$(LWIPDIR)/include
	NXDK_CFLAGS += -I$(NXDK_DIR)/lib/net/nforceif/include
	NXDK_CFLAGS += -I$(NXDK_DIR)/lib/net/nvnetdrv
	NXDK_CXXFLAGS += -I$(LWIPDIR)/include
	NXDK_CXXFLAGS += -I$(NXDK_DIR)/lib/net/nforceif/include
	NXDK_CXXFLAGS += -I$(NXDK_DIR)/lib/net/nvnetdrv

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.

[thrimbor]

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 nvnetdrv and nforceif directories, 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.

[thrimbor]

In this case it's actually fine, but other comments have spelling/grammar issues.

[thrimbor]

I'd go for _NXDK_XBE_TITLE, the leading underscore seems to be convention in CMake for variables that are considered internal to a module.
Could we also do an unset when we're done with it?