WIP: CMake Build System Changes

tomadamatkinson commented 9 months ago

This PR is currently a WIP. This PR has taken many shapes. This version of the PR intends to be less intrusive

CI has been altered to run the CMake version. This will be reverted in the future and is purely for testing - CI needs to exist in main to run.

TODO: [ ] Double check build vars are adhered too in CMake [ ] Explore vscode cmake extension usage [ ] Run tests in CI [ ] Fix windows CI [ ] Emscripten

wolfpld commented 9 months ago

CC @YaLTeR for comments about meson.

tomadamatkinson commented 9 months ago

@YaLTeR in terms of Meson I was thinking it might be simpler to wrap the cmake approach using the cmake module https://mesonbuild.com/CMake-module.html. That would reduce duplication and means that the project would only need to maintain a single build system.

Currently I believe there are 4 build systems (win, make, cmake, meson)

YaLTeR commented 9 months ago

I haven't used this meson CMake integration before, I guess we'll need to see how well it ends up working. Especially with regards to static/dynamic library, generated pkg-config and installing.

tomadamatkinson commented 9 months ago

In response to @wolfpld on #655

Vcpkg should only be used on windows, as a way to provide system libraries that you would have installed on unix.

vcpkg is installing only the dependencies that my system does not already have. The only platform i have not been able to test this on is MacOS

tomadamatkinson commented 9 months ago

Looks like i need to fixup the shared vscode config. Ill quickly do this so that the existing commands in launch.json work

wolfpld commented 9 months ago

vcpkg is installing only the dependencies that my system does not already have.

This is not the issue I am concerned with. Adding vcpkg adds yet another component to the build system that will inevitably break sooner or later. I don't want to be debugging why some tool I don't see the need for has stopped working and is breaking builds.

FWIW, you could probably just skip the vcpkg step on windows, since using whatever cmake provides to download and compile sources is functionally equivalent to how vcpkg is currently used (again, on windows).

Currently I believe there are 4 build systems (win, make, cmake, meson)

Yes, apparently adding a single source file to the build, which is the recommended method, is too difficult for some.

I am pushing closer to the automatic approach as most people that I have tried to persuade to adopt Tracy have failed due to their Linux distro not supporting the correct packages or broken packages which Tracy requires

Providing software packages is literally the only thing a Linux distribution does, and the way it does it is the only thing that differentiates them. The correct solution is to switch to a working distribution, not to insist that broken software be supported.

Looks like i need to fixup the shared vscode config. Ill quickly do this so that the existing commands in launch.json work

c_cpp_properties.json should be removed.

What you are trying to do with "compileCommands": "${workspaceFolder}/build/compile_commands.json" will not work, as you need to provide per-executable compile_commands.json to have proper defines set in VS Code. This is a hard requirement for this to go forward.

tomadamatkinson commented 9 months ago

c_cpp_properties.json should be removed.

I added this part as it looks like you had a shared .vscode folder. But the below shows that it was meant to be removed?

Should i just remove the .vscode folder in its entirety?

tomadamatkinson commented 9 months ago

FWIW, you could probably just skip the vcpkg step on windows, since using whatever cmake provides to download and compile sources is functionally equivalent to how vcpkg is currently used (again, on windows).

I originally planned on doing this however due to our conversation in the issue thread I decided to make it work. In all fairness vcpkg worked really well but will be easy to remove and fallback to CPM. CPM again doesnt need to be used but as its a single file include in CMake it helps reduce the amount of CMake needed to pull dependencies using fetch instead of submodules

tomadamatkinson commented 9 months ago

Removed vcpkg in favour of CPM

wolfpld commented 9 months ago

Should i just remove the .vscode folder in its entirety?

No, the executables must be launchable from VS Code and you need some configuration for that to happen. See the cmake branch.

tomadamatkinson commented 9 months ago

WASM was a little soul destroying but I got there :tada:

Changes specific for WASM support

emcmake cmake -Bbuild -S. -DCMAKE_BUILD_TYPE=Debug
cmake --build build --target profiler

Gives this in the binary folder

Had to add another header to httpd.py due to Chrome requiring specific requirements for COEP/COOP https://developer.chrome.com/blog/enabling-shared-array-buffer/)

tomadamatkinson commented 9 months ago

Added back .vscode/launch.json support. As the build is now using cmake it no longer needs to be platform specific. Because of this i renamed unix to native to make it less specific.

tracy__configure_target(target_name) can now be used to move a target from the CMake file structure to a more standard file structure. /bin /lib. This made hard coding the paths in lauch.json work on windows and unix

Below is the tracy test application build and run using the launch command

tomadamatkinson commented 9 months ago

Bonus: Launch Emscripten Application

Builds and compiles the WASM into build/emscripten/bin and runs using launch,json

tomadamatkinson commented 9 months ago

@YaLTeR Im not sure we need to touch the Meson as it looks like this only depends on public, common and client which i have left in place? The current implementation should work. It would be best to test this in a project which uses the Meson build system. Do any come to mind which I can test against?

tomadamatkinson commented 9 months ago

Build matrix latest run is mostly green https://github.com/tomadamatkinson/tracy/actions/runs/7381214753

This is missing running the test at the moment. I want to spend some time to make sure i get it right. But as we are using cmake i was able to condense the workflows into a single file. There are some meson issues when building on windows with ninja due to missing system libraries. Using vs automatically adds these

All successful release builds create zip artifacts named after the system they where build on

wolfpld commented 9 months ago

Some notes:

There is no way to choose which target you want to work with. The solution you provide only addresses building, which is not that important here. There needs to be a way to switch between targets from within VS Code to get the right set of defines in compilation_commands.json. If you put everything into a single CMake build, you get duplicate entries, e.g. for TRACY_NO_STATISTICS, which is not workable. I see I did not commit .vscode/settings.json to my branch cmake before (argh gitignores!), but that has been fixed. If you try to run with this file in place, you will see that you can now choose which CMake build configuration to use.

obraz

There is no pkg-config check for the availability of libraries like freetype or glfw before CPM is used. Capstone is fine as it is now, as discussed earlier.

You are relying on unsupported functionality with TBB:

[cmake] CMake Warning at build/_deps/tbb-src/CMakeLists.txt:111 (message):
[cmake]   You are building oneTBB as a static library.  This is highly discouraged
[cmake]   and such configuration is not supported.  Consider building a dynamic
[cmake]   library to avoid unforeseen issues.

TBB is not a Tracy dependency, but rather an artifact of how broken gcc and/or libstdc++ are. Basically, using certain parts of libstdc++ functionality requires linking to TBB, but neither gcc nor clang choose to automatically link to this library, which is their internal dependency, so you have to do it yourself. Of course, if the internals of libstdc++ change, or if you switch to a different STL implementation, these hidden requirements will change. It's a shit show and a prime example of things CMake should automate. I don't have any expectation that it will do so.
TBB must not be linked when compiling with MSVC, as it's not used for anything in that context.

tomadamatkinson commented 9 months ago

Hey @wolfpld,

Thanks for the feedback. I went down a whole rabbit hole yesterday! I appreciate you taking the time to look at it further

In terms of VSCode and CMake. Are you using a particular plugin or is this functionality built into Code by default? Most of the time I rely on tasks.json or the terminal to build and run apps. There is likely a way to fix this.

There should be two targets for the Tracy server. One with stats and one without, not sure how this effects the compile commands but I can take a look

There is no pkg-config check for the availability of libraries like freetype or glfw before CPM is used. Capstone is fine as it is now, as discussed earlier.

Yep agreed, I can add a check for system libs before falling back to CPM. I need to sort out the vendor CMake a little, it's a bit hacked together for now as I was trying to get most features working end to end

I also need to read all the Makefiles and Vsprojects again to make sure I have feature parity. It might be worth pulling these files back in at a later date. I deleted prematurely for my own readability

For TBB, I will follow your guidance. Looking for system libraries before using CPM might resolve this issue naturally

wolfpld commented 9 months ago

In terms of VSCode and CMake. Are you using a particular plugin or is this functionality built into Code by default? Most of the time I rely on tasks.json or the terminal to build and run apps. There is likely a way to fix this.

I use "CMake Tools" for this, which is developed by Microsoft and should have the most comprehensive features. I also use "CMake" from twxs, but I think it only provides CMake language support like syntax coloring or code completion.

For C++ language support I use the clangd extension. I find that this extension provides a better intellisense experience than the official Microsoft C/C++ extension.

For these intellisense type extensions to work properly, a build configuration must be known for each source file. This is basically the command line call to the compiler, with all the parameters such as includes, defines, language version, etc. This is done through the compile_command.json file, and works well. For the multi-build-dir solution you need to configure the "cmake.copyCompileCommands": "${workspaceFolder}/compile_commands.json" setting, in order to make this file visible to clangd, which uses a hardcoded path (sigh). I would also recommend setting "cmake.autoSelectActiveFolder": false. These should probably be placed in Tracy's settings.json.

There should be two targets for the Tracy server. One with stats and one without, not sure how this effects the compile commands but I can take a look

There are.

...
{
  "directory": "/home/wolf/tracy/build",
  "command": "/usr/sbin/clang++ -DNOMINMAX -DTBB_USE_DEBUG -I/home/wolf/tracy/server/.. -I/home/wolf/tracy/vendor -I/home/wolf/tracy/vendor/imgui -I/home/wolf/tracy/vendor/imgui_backend -I/home/wolf/tracy/vendor/zstd -I/home/wolf/tracy/build/_deps/tbb-src/src/tbb/../../include -isystem /home/wolf/tracy/public -isystem /home/wolf/tracy/build/_deps/glfw-src/include -isystem /home/wolf/tracy/build/_deps/capstone-src/include -isystem /home/wolf/tracy/build/_deps/nativefiledialog-extended-src/src/include -g -std=gnu++17 -o server/CMakeFiles/TracyServer.dir/TracyWorker.cpp.o -c /home/wolf/tracy/server/TracyWorker.cpp",
  "file": "/home/wolf/tracy/server/TracyWorker.cpp",
  "output": "server/CMakeFiles/TracyServer.dir/TracyWorker.cpp.o"
},
...
{
  "directory": "/home/wolf/tracy/build",
  "command": "/usr/sbin/clang++ -DNOMINMAX -DTBB_USE_DEBUG -DTRACY_ENABLE=0 -DTRACY_NO_STATISTICS=1 -I/home/wolf/tracy/server/.. -I/home/wolf/tracy/vendor -I/home/wolf/tracy/vendor/imgui -I/home/wolf/tracy/vendor/imgui_backend -I/home/wolf/tracy/vendor/zstd -I/home/wolf/tracy/build/_deps/tbb-src/src/tbb/../../include -isystem /home/wolf/tracy/public -isystem /home/wolf/tracy/build/_deps/glfw-src/include -isystem /home/wolf/tracy/build/_deps/capstone-src/include -isystem /home/wolf/tracy/build/_deps/nativefiledialog-extended-src/src/include -g -std=gnu++17 -o server/CMakeFiles/TracyServerNoStatistics.dir/TracyWorker.cpp.o -c /home/wolf/tracy/server/TracyWorker.cpp",
  "file": "/home/wolf/tracy/server/TracyWorker.cpp",
  "output": "server/CMakeFiles/TracyServerNoStatistics.dir/TracyWorker.cpp.o"
},
...

The problem is that you have two different build configurations for the same source file, and you need to be able to expose only one for the clangd extension.

The solution I came up with is to provide multiple and separate CMake build configurations, which are listed in settings.json in the cmake.sourceDirectory array. The CMake extension provides a way to switch between these configurations, so that you can work in the context of the GUI profiler, or in the context of the update utility, or in the context of the client integration code, etc.

tomadamatkinson commented 9 months ago

@wolfpld I've sorted out pulling system dependencies instead of fetching. If users prefer fetching they can set TRACY_FORCE_FETCH_DEPENDENCIES

I've been researching the CMake Tools extension. Although there is a separate issue with compile_commands.json which we might be able to overcome by using two CMakes. We might also be able to overcome this by using Variants https://github.com/microsoft/vscode-cmake-tools/blob/main/docs/variants.md

Another alternative for compile commands could be to have a TRACY_BUILD_TARGET which can be set to PROFILER, CLIENT or UTILS. That would allow us to have multiple build directories with different compile_commands.json. This approach might also work well with variants

tomadamatkinson commented 9 months ago

Variants might be a valid solution

I got this by adding a cmake-variants.json (can also be yaml)

We can then selectively expose targets and set options using

This should "solve" the problem. What do you think?

tomadamatkinson commented 9 months ago

OK ive implemented a possible solution for CMake Tools following the same principles as outlined above

Here you can see an option after configuring the Kit you can select a Variant

Options: [Debug | Release | RelWithDeb] -> [Static | Shared] -> [Client | Profiler | Utils]

Client is the default. CMake confirms its build mode with Selected Tracy build target: CLIENT as shown in the image below

Certain targets are now not accessible if Tracy is configured for a specific build type

Profiler -> profiler and csvexport (csvexport requires the server with stats where utils require it without) Client -> TracyClient Utils -> import-chrome, import-fuchsia, update and capture

I will need to fix-up CI and VSCode tasks

wolfpld commented 9 months ago

I looked at your branch again, sorry it took so long.

I am not able to launch with F5 from VS Code. I'm not sure the approach you're taking with launch.json and tasks.json is the right one, as you're basically overriding what the CMake extension should be doing. That is, you are providing launch configurations that directly depends on the build tasks in which you manually call CMake to do the job. If you look at my branch, you will see that there are no build tasks (those are automatically provided by the CMake extension), and the launch configuration is a template that retrieves data such as the binary path from the extension.

We should rely on what the extension provides, as the user will want to configure things like the build toolkit (gcc/clang/etc.) or target configuration (debug/release/etc.) from the extension UI, rather than relying on the hardcoded build tasks.

obraz

Then, by clicking on the "configure" entries, you will get these choices:

obraz

I'm not sure how emscripten would fit into this approach. I would assume that selecting emscripten as the build toolkit should be the way, but I don't know how to expose that build chain to CMake, or how to change the build configuration, to include the wget task and so on, depending on what you use to compile the binary.

Variants might be a valid solution

I got this by adding a cmake-variants.json (can also be yaml)

I don't think this is a proper solution to the problem at hand. Variants are used to solve different kinds of problems. For example, I would use variants to switch between a profiler UI that targets X and Wayland. Or to choose between an NFD backend implemented through the XDG portal or GTK.

By the way, please try adding these options while keeping the current defaults (Wayland + portals). The NFD library as it is currently built on your branch targets GTK.

Certain targets are now not accessible if Tracy is configured for a specific build type

Profiler -> profiler and csvexport (csvexport requires the server with stats where utils require it without) Client -> TracyClient Utils -> import-chrome, import-fuchsia, update and capture

I don't like it because it requires the user to have specific knowledge of how things are set up internally in the project. As you noted, csvexport stands out here in the wrong category, as it is definitely a utility. It feels like a hack.

I'm also not sure how to specify which executable to launch when I press F5. Note that this includes debugging in the IDE. Can you show me a sequence of commands you would need to do in VS Code to switch from profiler to update to the client and then back to profiler? With the solution I have on my branch, all I have to do is click on the currently selected "project" in the status bar and then select another one from the popup.

On another note, is there a reason for the NFD library to be fetched via CPM? It seems odd that you have singled it out while leaving zstd, dtl, etc. as-is. I'm asking out of curiosity, since NFD should just be vendored like all the other libraries. Having builds dependent on network availability is not something I want to have.

tomadamatkinson commented 8 months ago

By the way, please try adding these options while keeping the current defaults (Wayland + portals). The NFD library as it is currently built on your branch targets GTK.

This is on my TODO for this branch. I had a few days where i couldn't revisit but definitely on the radar.

I don't like it because it requires the user to have specific knowledge of how things are set up internally in the project. As you noted, csvexport stands out here in the wrong category, as it is definitely a utility. It feels like a hack.

It is definitely a hack. I will make a second comment to address this

I'm also not sure how to specify which executable to launch when I press F5. Note that this includes debugging in the IDE. Can you show me a sequence of commands you would need to do in VS Code to switch from profiler to update to the client and then back to profiler? With the solution I have on my branch, all I have to do is click on the currently selected "project" in the status bar and then select another one from the popup.

You can do this in the Run and Debug section in the top right of that panel. There is an option to switch between the run options.

On another note, is there a reason for the NFD library to be fetched via CPM? It seems odd that you have singled it out while leaving zstd, dtl, etc. as-is. I'm asking out of curiosity, since NFD should just be vendored like all the other libraries.

I chose to fetch this library as i was certain that there where no edits to its source. Other libraries that are still in vendor are only there due to my understanding that the source had been altered in some way. Using CPM allows us to selectively pull dependencies needed for a build. At the moment all dependencies are pulled but in the case of just building utilities we dont need to pull everything.

Having builds dependent on network availability is not something I want to have.

If you have network access to download Tracy you should have network access to pull submodules and/or use CPM to pull dependencies. CPM does allow us to only pull requred dependencies for specific builds so in effect the current approach can be configured to use less network. I would be astonished if a developer was attempting to compile any software without network especially with the amounts of dependencies software has these days

tomadamatkinson commented 8 months ago

It is definitely a hack. I will make a second comment to address this

The main goal of the suggested approach was to preserve the compile_commands.json so that for a given type of build we only had one set of compile commands for a file. See below

The problem is that you have two different build configurations for the same source file, and you need to be able to expose only one for the clangd extension.

csvexport is the only utility that depends on the server target being built with TRACY_NO_STATISTICS=OFF which meant that it had to be bundled with the profiler build. The 3 different TRACY_BUILD_TARGETS now should have correct compile_commands.json without duplicate entries for files.

I am not particularly happy with the approach but at the time of attempting to fix the problem this was the first solution that worked.

If we could fix TRACY_NO_STATISTICS where all executable can be compiled and work as expected without it then we can move back to a single build folder without TRACY_BUILD _TARGET and have a more traditional CMake setup with a correct compile_commands.json.

If it is feasible to always build with TRACY_NO_STATISTICS=OFF so that there is only one version of the server that needs to be compiled or that the other utilities where patched to work with TRACY_NO_STATISTICS ON or OFF. Then we might be able to go back to a single build folder and remove the TRACY_BUILD_TARGET complexity.

I guess a better question would be:

What is the true intended use case for TRACY_NO_STATISTICS and how does it fit into the different command line applications that are build by this project? Why do some depend on it being ON and some depend on it being OFF.

tomadamatkinson commented 8 months ago

In terms of:

What is the true intended use case for TRACY_NO_STATISTICS and how does it fit into the different command line applications that are build by this project? Why do some depend on it being ON and some depend on it being OFF.

The most i could find is this from the PDF docs

TRACY_NO_STATISTICS – Tracy will perform statistical data collection on the fly, if this macro is not defined. This allows extended trace analysis (for example, you can perform a live search for matching zones) at a small CPU processing cost and a considerable memory usage increase (at least 8 bytes per zone).

If this is the case do the utilities need to have TRACY_NO_STATISTICS=ON for them to work. At the moment if this isnt set they will not compile but I assume this is because they just need methods added for the TRACY_NO_STATISTICS=ON compile condition. I could add those with a runtime error to inform the user that they need to compile these executables with TRACY_NO_STATISTICS=ON which we could have configured in the variants for the CMake extension

wolfpld commented 8 months ago

You can do this in the Run and Debug section in the top right of that panel. There is an option to switch between the run options.

True. However, this is provided by the launch.json configuration. I have made some comments about this and am waiting for your response.

I chose to fetch this library as i was certain that there where no edits to its source. Other libraries that are still in vendor are only there due to my understanding that the source had been altered in some way.

ImGui is the only one where this would matter. I think zstd uses some static-linking-only functionality that prevents it from using shared libs provided by the distro packages.

If you have network access to download Tracy you should have network access to pull submodules and/or use CPM to pull dependencies.

There are two different aspects to this.

Using CPM for missing dependencies, which is fine because it's only used as a fallback. You can set up your system to provide these dependencies so that you can build without network availability.
Use CPM to get third-party code instead of committing it to the repository.

CPM does allow us to only pull requred dependencies for specific builds so in effect the current approach can be configured to use less network.

The problem is not the amount of network usage, but the lack of network availability, which will happen at the most inconvenient time.

I would be astonished if a developer was attempting to compile any software without network especially with the amounts of dependencies software has these days

Yeah, well, I don't want to be unable to compile my software because some DNS server was misconfigured or because someone forgot to renew their SSL certificate.

csvexport is the only utility that depends on the server target being built with TRACY_NO_STATISTICS=OFF which meant that it had to be bundled with the profiler build. The 3 different TRACY_BUILD_TARGETS now should have correct compile_commands.json without duplicate entries for files.

You have misunderstood. I'm not asking you to explain this to me, but to someone who needs to understand what's going on.

I could add those with a runtime error to inform the user that they need to compile these executables with TRACY_NO_STATISTICS=ON which we could have configured in the variants for the CMake extension

No. This creates friction for users. No one should have to manually change this to build an executable.

tomadamatkinson commented 8 months ago

Yeah, well, I don't want to be unable to compile my software because some DNS server was misconfigured or because someone forgot to renew their SSL certificate.

CPM is pulling from GitHub as far as i am aware. So that would be one hell of a DNS misconfiguration. I understand your concern but I am not sure that it is something that this project needs to solve. I am happy to add the source back at your request

wolfpld commented 8 months ago

Yes, please add NFD back.

tomadamatkinson commented 8 months ago

I've re-read the above to make sure this response is more concise. Apologies for the monologues. It helps me keep track of my decision making - definitely not trying to explain the obvious.

Am i correct in saying, the root issue of this solution is the dependency on TRACY_BUILD_TARGET. Would it be better placed to depend on TRACY_NO_STATISTICS as a variant instead. That would mean only exposing certain executables in the case where TRACY_NO_STATISTICS is ON or OFF?

wolfpld commented 8 months ago

Would it be better placed to depend on TRACY_NO_STATISTICS as a variant instead. That would mean only exposing certain executables in the case where TRACY_NO_STATISTICS is ON or OFF?

Yes, but that's also something I think we should avoid. You want to be switching between executables to build, not on some internal state that changes the set of available executables.

wolfpld commented 8 months ago

How would the current single CMakeLists.txt solution affect things for people that have it already integrated into their projects using the previously existing CMake solution?

People may rely on the CMakeLists.txt defining only the client part of the build and changes such as bumping the C++ standard, defining macros, or introducing new dependencies that may need to be downloaded from network may break things.

tomadamatkinson commented 8 months ago

Next time I can look at this is Thursday (possibly the weekend). My main goal is to iron out the developer experience. I agree with you this is too far from the original!

Also, a little ironic but I did experience an incident today where a demo didn't work because GitHub was down when attempting to pull source of third parties! So it might be best to stick to system dependencies unless pulling is explicitly set and of course re-add source as we discussed

wolfpld commented 8 months ago

Hah! Anyways, here's a bit of discussion on this topic: https://mastodon.gamedev.place/@wolfpld/111722945441538372

tomadamatkinson commented 8 months ago

Great read! You might not like yarn/npm or any web stack. 90% of code is pulled from a singular source (NPM) 😅

This is likely where my leniency for pulling third parties comes from. Most projects I work on build entirely from source and CI will pull that source when it builds. There are rarely any instances where a 5 minute PR cant fix a broken version or a hitch in the pipeline

That said, I am still onboard with finding the best compromises. I've had an opportunity to show alternative solutions to the CMake problem. They don't all need to stick 😄. Excited to see where this PR goes. Hopefully it's not getting on your nerves too much!

tomadamatkinson commented 7 months ago

Apologies for my absence. Work has been busy! Hoping to pick this one back up over the weekend

wolfpld commented 7 months ago

No worries, things happen.

tomadamatkinson commented 6 months ago

Hey @wolfpld. Apologies again for the delay!

Due to the original PR taking the wrong direction in terms of the project goals I decided to start again. This version is closer to your original suggestion and drops the idea of a single cmake build system entirely. I also made sure to not delete the makefiles - originally this was useful for me to understand the project but I can see how that would make reviewing a nightmare!!!

TRACY_BUILD_TARGET has been completely removed

There are still a few issues with Windows and my CMake doesnt conform 1:1 with the compile defines you have in Makefiles. My goals in the coming week will be to make sure this version matches the makefiles and that the extension usage we discussed above also works

I have highjacked the current linux.yml workflow so that I could test out the CI. In the long run the original CI will stay and there will be a cmake.yml workflow for the cmake stuff

tomadamatkinson commented 6 months ago

If you add these options to .vscode/settings.json the CMake Tools extension seems to work. I don't actually use this extension in practice as I prefer the command line. Please point out if this is still not as expected and what the expected flow would be :)

"cmake.buildDirectory": "${workspaceFolder}/build",
"cmake.sourceDirectory": [
    "${workspaceFolder}/capture",
    "${workspaceFolder}/profiler",
    "${workspaceFolder}/csvexport",
    "${workspaceFolder}/import-chrome",
    "${workspaceFolder}/import-fuchsia",
    "${workspaceFolder}/update",
]

wolfpld commented 6 months ago

I have adapted parts of this PR to the cmake branch. The branch compiles on Linux and Windows (using the MSVC toolchain), and it exposes build options that were previously available. It also handles some nuances not covered by this PR. Currently, only the profiler tool is buildable. Work remains to enable building of other tools.

Other than that, the following two things must be working before a merge to master:

Emscripten support, producing a set of files to be uploaded to the server, as it is now.
Properly handle the generation of Wayland protocol sources. Currently, there are some source files in profiler/src/wayland that need to be automatically generated. An equivalent solution in meson looks like this:

wl_proto = dependency('wayland-protocols')
wl_scanner = dependency('wayland-scanner')

wl_proto_dir = wl_proto.get_variable('pkgdatadir')
wl_scanner_bin = find_program(wl_scanner.get_variable('wayland_scanner'))

protocols = {
    'xdg-shell': wl_proto_dir / 'stable/xdg-shell/xdg-shell.xml',
    'xdg-decoration': wl_proto_dir / 'unstable/xdg-decoration/xdg-decoration-unstable-v1.xml',
}

foreach name, path : protocols
    code = custom_target(
        name.underscorify() + '_c',
        input: path,
        output: '@BASENAME@-protocol.c',
        command: [wl_scanner_bin, 'private-code', '@INPUT@', '@OUTPUT@']
    )
    files += code

    client_header = custom_target(
        name.underscorify() + '_client_h',
        input: path,
        output: '@BASENAME@-client-protocol.h',
        command: [wl_scanner_bin, 'client-header', '@INPUT@', '@OUTPUT@']
    )
    files += client_header
endforeach

This needs to be replicated in CMake.

Review and further help would be appreciated.

wolfpld commented 6 months ago

Ok, I have pushed further and now all the tools can be compiled both on Linux and Windows.

Some random notes:

You have changed CMAKE_CURRENT_SOURCE_DIR to CMAKE_CURRENT_LIST_DIR in the root CMakeLists.txt. Why?
You have modified macro (set_option... in the same file. Why?
What is the significance of minimal CMake version set to 3.19?
Option to select the linker will only be available in CMake 3.29 (https://gitlab.kitware.com/cmake/cmake/-/merge_requests/8861).
Windows builds are not parallel.

tomadamatkinson commented 6 months ago

Thanks for taking a look, I plan to work on the emcsripten toolchain and the Wayland function later today.

In terms of your recent questions:

I found that using current source dir gave incorrect paths depending on which file included the server cmake. Using list dir made sure the path was as I expected

No significance to the minimum version, we should be able to lower this. Most systems now ship with 3.19 or higher afaik (I'd have to double check android studio a few years a go it got a bump to 3.18)

Windows builds should be parallel if you build with --parallel or -j N. Those options have worked for me in the past. I can double check later though. Docs say that these map to native parallelism flags

wolfpld commented 6 months ago

I found that using current source dir gave incorrect paths depending on which file included the server cmake. Using list dir made sure the path was as I expected

The server file, sure, but I am asking about the build file in the root of the repository, which you would use to integrate Tracy with applications, and which is not referenced by any other build file.

Windows builds should be parallel if you build with --parallel or -j N. Those options have worked for me in the past. I can double check later though. Docs say that these map to native parallelism flags

I'm doing this through the VS Code extension and it doesn't seem to work no matter what I enter into the parallel build entry field. The default value of 0 is supposed to be a reasonable default. It works as intended on Linux. Supposedly there are some magic values you can input on the command line to setup the MSVC generator to do the correct thing, but it's just a hack instead of a proper solution.

From what I have found with a cursory look on the Internet, they seem to be confusing parallel building of multiple projects with parallel building of source files within a single project. It's a bit disappointing.

tomadamatkinson commented 6 months ago

We could append the required parallel compile flags if MSVC through CMake. I'll have to have a look when I'm at my desk but I believe it's something like "/MP"

wolfpld commented 6 months ago

List of remaining things:

Parallel compilation on MSVC seems to be a core CMake issue, not something we can or should fix.
Linking with the mold linker will be possible with CMake 3.29.
I did some work to enable Emscripten through the emcmake wrapper and it does all the motions it should do, but doesn't link. Trying to manually compile capstone to see if the old build system still works results in the same problems, so this is something broken in Emscripten and I don't have the energy to deal with that bullshit right now.
CI needs to be adapted (I have not looked closely at what you did with it yet).
Manual needs to be updated with build instructions, a VS Code section explaining how to do things would also be nice.
Macos has been verified to work.

Overall, I'd say the merge is very close now.

tomadamatkinson commented 6 months ago

In terms of CI and build docs. The CI I added can be moved to its own pipeline. I piggy backed on existing yml's in order for it to run.

I intend on keeping the original CI. My workflow should generate a zip for each platform as an artifact - not necessary but a nice to have

I'm assuming build docs just need a CMake section. Alongside the existing docs?

wolfpld commented 6 months ago

The old build system is already removed on the cmake branch. I will take care of CI and the docs.

wolfpld commented 6 months ago

It is done.

wolfpld / tracy

WIP: CMake Build System Changes #707