tket icon indicating copy to clipboard operation
tket copied to clipboard

Cmake improvements and Nix unification with existing pytket setup

Open jake-arkinstall opened this issue 7 months ago • 6 comments

Description

Presently, TKET's CMakeLists.txt declares its dependencies as private, even when they are depended upon in public headers. As such, CMake projects that import TKET as a dependency must also add those dependencies in order to #include <tket/Circuit/Circuit.hpp>, for example.

In this PR, such dependencies are now declared public in the target_link_libraries. This is also done for libs/tktokenswap and libs/tkwsm, such that projects depending on them should now need to add no additional dependencies.

With these changes in place, the CMakeLists.txt for tests and for pytket could be simplified, and this is done in this PR too. This provided an opportunity to simplify the Nix build process, and this has been done. Nix now builds pytket through the CMake path in setup.py instead of the custom NixBuild path. This does require some minor changes to the CMakeBuild, but I believe they are inconsequential to non-nix builds (CI will verify):

  • A user can now pass a BUILD_DIR environment variable to setup.py to specify where to build the cmake project in setup.py. This is because the rmtree command was also removing python setup outputs within a nix build, as those happened to also be placed inside build/. By providing e.g. a temporary directory, Nix can avoid this problem while non-nix users still have their builds default to the build directory.
  • Instead of removing the build directory and then making it, it is now simply stripped of existing files. This helps in the situation when a BUILD_DIR is provided in the environment with insufficient removal permissions, and also avoids a potential problem wherein a terminal session open in the build directory becomes homeless, rather than simply observing files being removed.

tests/CMakeLists.txt is also simplified by globbing test files, rather than listing them manually. This is an opinionated change.

There was an inconsistency in #include styles within headers. Files were sometimes included by name only, sometimes by a relative path, and sometimes as relative to the respective project root. I have modified these to follow the 'respective project root' approach, as this is the most explicit, and most tooling-friendly. However, this introduces a lot of small changes in a lot of files, and may introduce merge conflicts if incoming PRs might make header adjustments in affected regions. As such, it might be preferable to eliminate those changes.

Related issues

#1482

Checklist

  • [x] I have performed a self-review of my code.
  • [x] I have commented hard-to-understand parts of my code.
  • [ ] I have made corresponding changes to the public API documentation.
  • [x] I have added tests that prove my fix is effective or that my feature works.
  • [ ] I have updated the changelog with any user-facing changes.

jake-arkinstall avatar Jul 25 '24 10:07 jake-arkinstall