Skip to content

WIP: CMake Build System Changes#707

Closed
tomadamatkinson wants to merge 6 commits intowolfpld:masterfrom
tomadamatkinson:buildsys
Closed

WIP: CMake Build System Changes#707
tomadamatkinson wants to merge 6 commits intowolfpld:masterfrom
tomadamatkinson:buildsys

Conversation

@tomadamatkinson
Copy link
Copy Markdown
Contributor

@tomadamatkinson tomadamatkinson commented Jan 1, 2024

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
Copy link
Copy Markdown
Owner

wolfpld commented Jan 1, 2024

CC @YaLTeR for comments about meson.

@tomadamatkinson
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Contributor

YaLTeR commented Jan 1, 2024

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
Copy link
Copy Markdown
Contributor Author

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.

image

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
Copy link
Copy Markdown
Contributor Author

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

@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Jan 1, 2024

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
Copy link
Copy Markdown
Contributor Author

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?

image

@tomadamatkinson
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor Author

Removed vcpkg in favour of CPM

image

@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Jan 1, 2024

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
Copy link
Copy Markdown
Contributor Author

WASM was a little soul destroying but I got there 🎉

Changes specific for WASM support

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

Gives this in the binary folder

image

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
Copy link
Copy Markdown
Contributor Author

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

image

@tomadamatkinson
Copy link
Copy Markdown
Contributor Author

Bonus: Launch Emscripten Application

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

image

@tomadamatkinson
Copy link
Copy Markdown
Contributor Author

@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 tomadamatkinson force-pushed the buildsys branch 14 times, most recently from 2a1b1cf to 8668fc9 Compare January 2, 2024 00:41
@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Jan 9, 2024

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

wolfpld commented Jan 9, 2024

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

@tomadamatkinson
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor Author

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

@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Feb 23, 2024

No worries, things happen.

Temp: Remove CI

linux dependencies

auto download dependencies

install libs with choco

name artifacts

use LINK_LIBRARIES

add concurrency

install dependencies

install dependencies

install dependencies

passwordless sudo

windows fix

more windows fixes

more windows fixes

no statistics
@tomadamatkinson
Copy link
Copy Markdown
Contributor Author

tomadamatkinson commented Mar 10, 2024

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

wolfpld commented Mar 16, 2024

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
Copy link
Copy Markdown
Owner

wolfpld commented Mar 17, 2024

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

wolfpld commented Mar 17, 2024

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

wolfpld commented Mar 21, 2024

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

wolfpld commented Mar 21, 2024

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

@wolfpld
Copy link
Copy Markdown
Owner

wolfpld commented Mar 23, 2024

It is done.

@wolfpld wolfpld closed this Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants