Clean CMake

[clementperon]

Hi @m3bm3b,

I'm trying to build onnxruntime with Yocto on RISCV32 arch.
ONNXRuntime use NSYNC and unfortunately it doesn't properly build at the moment:

Due to a QA Check warning on some absolute path.
Error is:
| CMake Error in /home/nobuo/Data/yocto/main/riscv-build/tmp-glibc/work/riscv32-oe-linux/onnxruntime/1.18.0/build/_deps/google_nsync-src/CMakeLists.txt:
| Target "nsync_cpp" INTERFACE_INCLUDE_DIRECTORIES property contains path:
|
| "/home/nobuo/Data/yocto/main/riscv-build/tmp-glibc/work/riscv32-oe-linux/onnxruntime/1.18.0/build/_deps/google_nsync-src/public"
|
| which is prefixed in the build directory.

This is not mandatory and I could bypass it but I prefer to clean, to avoid any issue in case I need to build an SDK.

The issue seems to come from ONNXRuntime and how they include nsync in there project.
https://github.com/microsoft/onnxruntime/blob/main/cmake/CMakeLists.txt#L1048
https://github.com/microsoft/onnxruntime/blob/main/cmake/CMakeLists.txt#L1179C10-L1187

This seems to be required because NSYNC use path relative to PROJECT_SOURCE_DIR which regarding how you include this project may fail.

I propose in this MR to clean and drop CMake below 3.5 but only the last commit should be usefull.
I have also used cmake-format to reindent the CMake.

Let me know what you think,
Thanks

[clementperon]

Also I don't have a way to test all the machine, but would you be ok for a Github CI workflow ?

[m3bm3b]

I'm still testing on various platforms (which might take a while), but the changes in the last commit ("avoid using absolute path and introduce copy_to_cpp function") seem like a good idea.

I'm not keen on the other commits:

1. While I usually like standardized formatting and automatic formatting tools, cmake-format seems to hide the underlying semantic structure, rather than reveal it. For example, most "set" calls act as though they have two parameters: a variable name, and a list of values. As a result, I think it helps to format things that way, but cmake-format treats "set" as though it takes a flat list, which loses the structure. I fiddled with cmake-format's options, but no matter what I do, I can't make it retain that sort of structure information. So I do not wish to use cmake-format. The only formatting change I'd suggest is to run the file through "unexpand" to eliminate the few places where spaces rather than tabs have been used for indentation.

2. I don't strenuously object to removing the parameter from endfunction(), but I don't understand the benefit of the change. It's legal, and is consistent with other places in the file where similar optional parameters are used.

3 and 4. I would prefer to continue supporting versions of cmake as far back as is practical. Is there a benefit in using Thread::Thread now? Or can it wait until new cmakes no longer support v2.8.12 syntax?
For clients, it's almost always better if maintainers keep things stable and compatible unless there's a good reason.

[m3bm3b]

Could we proceed with just your last commit?

[clementperon]

Could we proceed with just your last commit?

After testing it still doesn't fix my issue, I need to investigate a bit more.
I suspect the target_include_directories that need to have an explicit BUILD_INTERFACE and INSTALL_INTERFACE.

Let me finish to understand properly that and I will remove the format and the drop of CMake 2.x

[m3bm3b]

Ok. Let me know.

By the way, while you're changing CMakeLists.txt, would you change the first line to this:
cmake_minimum_required (VERSION 2.8.12...3.28.3)

I believe that tells cmake it will work with any version in that range, and it stops it from warning about the
future deprecation of the old version.
I've tried the current CMakeLists.txt with 3.28.3 version of cmake, and it seems to work.
But if you'd rather not do that change, I can do it in a later commit.

Thanks.

[m3bm3b]

Previously when I built using cmake,
the build process did not change the source directory.
It created a "cpp" directory, but in the build directory.

But with this new CMakeLists.txt, it creates a subdirectory called "cpp" in the top-level source directory!
cmake is supposed to keep the build-state separate from the source, so it shouldn't be changing the source directory.
Even worse, it's using a name ("cpp") that is independent of the target name, so two concurrent builds
(either on the same machine or on different machines) can be trying to write the same files at the same time
within the source directory. I see no way to guarantee that that is safe, given that various different
file system protocols may be in use concurrently.

Is there some reason why it cannot work to create "cpp" in the build directory?

[m3bm3b]

Based on how cmake handles yacc, which also generates some C source from other source,
I suspect the "cpp" directory should be in ${CMAKE_CURRENT_BINARY_DIR} rather than ${CMAKE_CURRENT_SOURCE_DIR}.

[m3bm3b]

Looks good. I tested it with an old and a new cmake.

Can you confirm that your verified that this latest version fixes the issue you were having with ONNXRuntime with riscv32?

[clementperon]

Looks good. I tested it with an old and a new cmake.

Can you confirm that your verified that this latest version fixes the issue you were having with ONNXRuntime with riscv32?

@m3bm3b no sorry I was busy with other tasks, I hope I could manage this this weekend.