cmake: Auto discover NXDK_DIR

[abaire]

Simplifies creation of projects in CLion

[RadWolfie]

@abaire, have you tested this without NXDK_DIR environment variable set? I am a bit concerned it may not work accurately as it could mix up between installed and portable nxdk folders. Plus the fact that environment variable isn't set at all will cause nxdk's compiler to fail (maybe?).

[abaire]

I've only tested it without the NXDK_DIR set as the entire intent here is to avoid having to set the env var :)

The toolchain file redefines all of the standard include paths based on its notion of the NXDK_DIR, which obviates the ones in the wrapper scripts:

nxdk/share/toolchain-nxdk.cmake

Line 26 in 2aab910

# Keep in sync with include paths in bin/nxdk-cc

This change would clearly be problematic if somebody explicitly wanted to use the toolchain file from an install of the nxdk at location A with the wrapper scripts from location B, but that seems like an obviously incorrect/misleading use case to me. Is there a reason someone would want to mix and match parts of two nxdk installs rather than just use the entire nxdk bundle?

[RadWolfie]

No one wants to mix and match parts of two nxdk folders. 🥲 Even though I do see the mistake occur in upstream's cmake will redirect to environment variable setter's nxdk folder. However this change from my view could cause even more mix up of using two nxdk folders. Since you have tested without environment variable and able to build an xbe successfully. The concern can be dismissed for this part.

One more question, did you test any of linkage library inside pkgconfig folder? Since they use environment variable too.

# Keep in sync with include paths in bin/nxdk-cc
...

For whatever reason with IDE(s?, in my case, visual code) always show either blue or red underline names or unable to show list of possible matched names.

Is there a reason someone would want to mix and match parts of two nxdk installs rather than just use the entire nxdk bundle?

As for your question, some of the people in the community do use nxdk as a submodule (which can be found on GitHub repos) for some odd reason. So, having 2+ nxdk folders for 2+ projects could have one of two folders being used as NXDK_DIR environment set. Plus the user may have the main nxdk installed as well (as NXDK_DIR environment set too). I can see these as mostly user errors. Yet a valid reason for such a thing to exist is experimental forked nxdk to verify improvement/feature works in their fork before create a pull request. One other possible reason is to choose a specific commit that is stable for their project(s). What I'm trying to say is having two separate nxdk folders is intended to verify the code works from their respective nxdk folder rather than some files accidentally included from another nxdk folder unintentionally.

I apologize for the rambling I may have made above. I tried to keep it short and simple. Yet it got longer and complex than I expected.

[abaire]

One more question, did you test any of linkage library inside pkgconfig folder? Since they use environment variable too.

I've not tested it enough to have high confidence, so converting this to draft. I believe pkgconfig is only used at configuration time, so we could probably just set the NXDK_DIR environment within the toolchain and it'd do the right thing.

I'll set up a test suite so the behavior can be confirmed, but thinking about this more I feel like going all-in on a CMake path might be less troublesome than trying to support the Makefile case and CMake with shared scripts. I'll eventually do some testing of that on my own to see how much duplication it'd cause.

For whatever reason with IDE(s?, in my case, visual code) always show either blue or red underline names or unable to show list of possible matched names.

I suspect it is because they are unaware of the env var expansion when doing indexing. CLion does resolve this properly (both in the env var case, provided you jump through the hoops to set the env var within the IDE, and this PR).

Is there a reason someone would want to mix and match parts of two nxdk installs rather than just use the entire nxdk bundle?

As for your question, some of the people in the community do use nxdk as a submodule (which can be found on GitHub repos) for some odd reason. So, having 2+ nxdk folders for 2+ projects could have one of two folders being used as NXDK_DIR environment set. Plus the user may have the main nxdk installed as well (as NXDK_DIR environment set too). I can see these as mostly user errors. Yet a valid reason for such a thing to exist is experimental forked nxdk to verify improvement/feature works in their fork before create a pull request. One other possible reason is to choose a specific commit that is stable for their project(s). What I'm trying to say is having two separate nxdk folders is intended to verify the code works from their respective nxdk folder rather than some files accidentally included from another nxdk folder unintentionally.

I use the NXDK as a submodule in several projects to allow project-specific patches that wouldn't make sense to upstream. I think the valid cases that you cite are the same as my use case; it would clearly be an error to have the NXDK_DIR set to one place (e.g., a "main" nxdk install) and use the toolchain from the submodule (and would arguably invalidate their feature testing if they're changing the nxdk itself).

I think in principle it should always be the case that the toolchain is loaded from ${NXDK_DIR}/share (in fact that's what the nxdk-cmake wrapper does) so I think it is reasonable to use toolchain-relative paths for all CMake tooling.

I'm making the assumption in this PR that the env var was primarily created to reduce duplication between the CMake and Makefile paths. Since there's already a requirement to keep things in sync between the toolchain and the wrappers, it feels like entirely separating them in order to remove the need to modify the environment is a reasonable tradeoff. It would mean more stuff needs to be kept in sync on the maintenance side, but presumably there are many times as many projects being set up and built than there are changes to the core flags/tools?

[RadWolfie]

I've not tested it enough to have high confidence, so converting this to draft. I believe pkgconfig is only used at configuration time, so we could probably just set the NXDK_DIR environment within the toolchain and it'd do the right thing.

In my opinion, I agreed. Yet rather have it tested first to be sure. :)

I'll set up a test suite so the behavior can be confirmed, ...

👍

it would clearly be an error to have the NXDK_DIR set to one place (e.g., a "main" nxdk install) and use the toolchain from the submodule (and would arguably invalidate their feature testing if they're changing the nxdk itself).

Agreed, although cmake does revert the environment set that was done from within. Source of someone who did a basic test: https://stackoverflow.com/a/67657534 My opinion is having an environment variable set is a standard way to do things. Cmake's variables are only accessible from within cmake tool and cause conflict for external tools that require the supplied variable to function correctly. What made me not confident with this pull request change was the absence of an environment variable setter. I'm not even sure why I didn't use environment variable setter in my cmake "bugfixing" pull request to ensure nxdk's cmake tool and compiler/linkage/whatever are using the same path as nxdk folder is targeted.

It would mean more stuff needs to be kept in sync on the maintenance side, but presumably there are many times as many projects being set up and built than there are changes to the core flags/tools?

I don't have the answer for your last question sadly.

[abaire]

Added a trivial build test. You were correct that the pkg-config use of NXDK_DIR needed to be accounted for, the toolchain file now sets the environment var allows pkg_check_modules to work during the configuration pass, as expected. I believe this also guarantees that the NXDK_DIR-relative paths hardcoded into the wrapper scripts will be the same as the toolchain provided (and with this PR, toolchain-relative) ones.

Agreed, although cmake does revert the environment set that was done from within.

Provided the user isn't running arbitrary commands outside of CMake and expecting them to work without setting NXDK_DIR, I think this is OK? Modification to the CMake env is presumably only valid from within the CMake invocation itself, so I'd expect having a CMakeLists/whatever modify the environment would not leak, which seems to be what the answer to that SO post indicates.

For clarity: if a user needs to invoke the wrapper scripts directly they would still need to run activate/set the env var as today. I'm not proposing that the env var be removed from the wrappers, rather that the CMake path should ideally not require a special environment, since it should be capable of fully configuring itself based via the toolchain.

[abaire]

Added a few .pc files to cover the nxdk components that aren't automatically added in the toolchain to provide a unified mechanism to pull in everything.

I think ideally the "tests" stuff could live under .github and be invoked on PRs to verify that CMake-based building works in the same way that .ci_build_samples.sh verifies the make path

[RadWolfie]

I like the idea of using pkgconfig for the optional nxdk components. I'm not sure if the net component will be permanent until someone upstream support of winsock library. However, a temporary workaround is better to have than waiting for winsock library is implemented.

As for winmm library, as far as I remember on Windows, it should be done by the user. Either by #pragma comment(lib, "winmm.lib") in the source code or target_link_libraries(<your_project_name or ${PROJECT_NAME}> winmm.lib) call in CMake. So, it might be fine for a temporary workaround if unable to perform the two methods that I am familiar with until a permanent fix is made.

I think ideally the "tests" stuff could live under .github and be invoked on PRs to verify that CMake-based building works in the same way that .ci_build_samples.sh verifies the make path

I would not necessarily agree with "tests" stuff residing in the .github folder as there's a possibility contributor could run the test project(s) locally to be sure nothing is broken. The samples folder is in the root directory and the tests folder should be in the root directory to be easily recognized we do have sample/test projects that can be vetted for no breakage within nxdk tools.

---

In my opinion, this pull request is no longer cmake: Auto discover NXDK_DIR modification. Perhaps a change to the PR title or split them into separate pull requests. As this pull request is pretty much QoL improvements and added the missing stuff that wasn't there before.

[abaire]

As for winmm library, as far as I remember on Windows, it should be done by the user. Either by #pragma comment(lib, "winmm.lib") in the source code or target_link_libraries(<your_project_name or ${PROJECT_NAME}> winmm.lib) call in CMake. So, it might be fine for a temporary workaround if unable to perform the two methods that I am familiar with until a permanent fix is made.

I don't think pkg-config does anything magic so it'd still require user action to wire it in. The pc file just allows you to glob nxdk features together in your CMakeLists if desired, you could also explicitly depend on winmm.lib with the appropriate linker path settings.

I would not necessarily agree with "tests" stuff residing in the .github folder as there's a possibility contributor could run the test project(s) locally to be sure nothing is broken. The samples folder is in the root directory and the tests folder should be in the root directory to be easily recognized we do have sample/test projects that can be vetted for no breakage within nxdk tools.

I'd argue that samples are examples of how the nxdk is meant to be used whereas tests are there to validate changes to the project itself. In particular I would not want somebody to look at the hacky test target and think that that is the best way to set up a CMake-based project using the nxdk. I think the bar to finding that the tests exist and how to run them is pretty low for anybody doing a PR (if you break the test I believe the failure will provide sufficient info to discover and run it locally).

I'll leave it for the maintainers to chime in on their preference of location. I'll hold off on changing the workflow to include the tests until it's agreed that these tests are desirable at all.

In my opinion, this pull request is no longer cmake: Auto discover NXDK_DIR modification. Perhaps a change to the PR title or split them into separate pull requests. As this pull request is pretty much QoL improvements and added the missing stuff that wasn't there before.

Yeah, it's expanded a bit. I'll again let a maintainer decide if it's worth renaming or decomposing the commits further.

Thanks for your feedback, I had missed that the package config stuff existed and wouldn't have found it if you hadn't asked :)