Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake improvements and Nix unification with existing pytket setup #1501

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jake-arkinstall
Copy link
Contributor

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

  • I have performed a self-review of my code.
  • I have commented hard-to-understand parts of my code.
  • I have made corresponding changes to the public API documentation.
  • 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 jake-arkinstall requested a review from cqc-alec July 25, 2024 10:55
@jake-arkinstall jake-arkinstall linked an issue Jul 25, 2024 that may be closed by this pull request
ext_suffix = get_config_var("EXT_SUFFIX")
lib_names.extend(f"{binder}{ext_suffix}" for binder in binders)
lib_names = [f"{binder}{ext_suffix}" for binder in binders]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have removed libtket.so from lib_names here, is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is - the binders are already linked to tket.

So if I run nix build .#pytket and it outputs, say, /nix/store/afanh2nndhwmnjx393pwry7di3gs0gf8-python3.12-pytket-1.30.0, I get:

find /nix/store/afanh2nndhwmnjx393pwry7di3gs0gf8-python3.12-pytket-1.30.0/ -name "*.so" | xargs ldd
/nix/store/afanh2nndhwmnjx393pwry7di3gs0gf8-python3.12-pytket-1.30.0/lib/python3.12/site-packages/pytket/_tket/architecture.cpython-312-x86_64-linux-gnu.so:
	linux-vdso.so.1 (0x00007ffebeffa000)
	libtket.so => /nix/store/r83mq87lf2jyzdzhb25v9kslqf1ccdgk-tket/lib/libtket.so (0x00007fef3c200000)
	libsymengine.so.0.11 => /nix/store/fww6693dlgs4fxcfcaib2md8w7sfp6r2-symengine-0.12.0/lib/libsymengine.so.0.11 (0x00007fef3ba00000)
	libflint.so.17 => /nix/store/bpryc1gw9a15nmdk3036jmdgqh4y0ix0-flint-2.9.0/lib/libflint.so.17 (0x00007fef3ae00000)
	libmpc.so.3 => /nix/store/s2cb7sfy1l5d7j1c0dryk4j4spq5adp4-libmpc-1.3.1/lib/libmpc.so.3 (0x00007fef3cc12000)
	libmpfr.so.6 => /nix/store/2wrknbiwjkrhywgfzc2wjd273bghv0xb-mpfr-4.2.1/lib/libmpfr.so.6 (0x00007fef3cb5a000)
	libgmp.so.10 => /nix/store/ip431mx66mcnbbzfphfzvr1h61vddc2q-gmp-with-cxx-6.3.0/lib/libgmp.so.10 (0x00007fef3cab5000)
	libtktokenswap.so => /nix/store/ylz20wz0q4pi8k9fcvaap867ddyhm1r2-tktokenswap/lib/libtktokenswap.so (0x00007fef3c185000)
	libtkassert.so => /nix/store/hjm2nfl2k8s6i78a7a080r42pld268f0-tkassert/lib/libtkassert.so (0x00007fef3caaf000)
	libtklog.so => /nix/store/vqgqfqwkhbbvgibiifhfi0ppwayf6wrw-tklog/lib/libtklog.so (0x00007fef3caa6000)
	libtkrng.so => /nix/store/l2k1id5xa9x6ps6difknkkfyx2fdgd4q-tkrng/lib/libtkrng.so (0x00007fef3caa0000)
	libstdc++.so.6 => /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libstdc++.so.6 (0x00007fef3aa00000)
	libm.so.6 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libm.so.6 (0x00007fef3c09f000)
	libgcc_s.so.1 => /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libgcc_s.so.1 (0x00007fef3c07a000)
	libc.so.6 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libc.so.6 (0x00007fef3a809000)
	libtkwsm.so => /nix/store/8s49jk9li42z583k2qgiyrpzvkdd7bh5-tkwsm/lib/libtkwsm.so (0x00007fef3b978000)
	libopenblas.so.0 => /nix/store/iy0g9kndccxpknqk8flsvzhqzjni26rd-openblas-0.3.27/lib/libopenblas.so.0 (0x00007fef38da8000)
	libntl.so.44 => /nix/store/sm8zhkpz3rrdldfknm4w5zz994lvsrfg-ntl-11.5.1/lib/libntl.so.44 (0x00007fef38a00000)
	libpthread.so.0 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libpthread.so.0 (0x00007fef3ca97000)
	/nix/store/dbcw19dshdwnxdv5q2g6wldj6syyvq7l-glibc-2.39-52/lib64/ld-linux-x86-64.so.2 (0x00007fef3cca4000)
	libgfortran.so.5 => /nix/store/wmina0mbakp0vkv4amkha9sv8zj9bkx2-gfortran-13.3.0-lib/lib/libgfortran.so.5 (0x00007fef38600000)
	libgomp.so.1 => /nix/store/c6r62m84hywf4i6qq1h28f13zv38yqyp-gcc-13.3.0-lib/lib/libgomp.so.1 (0x00007fef3c02d000)
	libgf2x.so.3 => /nix/store/bx8mvbs9w946xs5csfh5ijl43ggd98p8-gf2x-1.3.0/lib/libgf2x.so.3 (0x00007fef3b95c000)
	libdl.so.2 => /nix/store/m71p7f0nymb19yn1dascklyya2i96jfw-glibc-2.39-52/lib/libdl.so.2 (0x00007fef3ca90000)

I believe this to be sufficient, as the cmake builds of pytket are working on nix, ubuntu and macos.

The windows conan build, however, is failing, and and I'm scratching my head over that as the changes to the conan build are trivial.

@jake-arkinstall jake-arkinstall force-pushed the 1482-investigate-potential-cmake-improvements branch from 4913ad0 to 9abb317 Compare July 26, 2024 15:57
@cqc-alec
Copy link
Collaborator

Thanks Jake, looks good. When testing locally I encountered a couple of issues (following my usual local workflow).

First, when building tket with BUILD_TKET_TEST set:

cd tket
mkdir build
cd build
cmake -DCMAKE_INSTALL_PREFIX=/home/alec/local -DBUILD_TKET_TEST=1 ..
cmake --build . -j10

I get linker errors, seemingly related to gmp (which I have installed).

Secondly. when trying to build pytket and run the tests (after first building shared-library versions of tklog and tket):

cd pytket
NO_CONAN=1 INSTALL_DIR=/home/alec/local PYTKET_CMAKE_N_THREADS=10 pip install -v -e .[zx]
cd tests
pytest

I get a lot of errors like this:

____________________________ ERROR collecting tests/add_circuit_test.py _____________________________
ImportError while importing test module '/home/alec/r/tket/pytket/tests/add_circuit_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
add_circuit_test.py:15: in <module>
    from pytket import Circuit, OpType
../pytket/__init__.py:17: in <module>
    from pytket.circuit import (
../pytket/circuit/__init__.py:29: in <module>
    from pytket._tket.circuit import *
E   ImportError: /home/alec/r/tket/pytket/pytket/_tket/libtket.so: undefined symbol: __gmpn_perfect_square_p

Again this seems gmp-related.

Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issue with the changes; a merge conflict needs resolving; and I'd like to understand why my usual local methods of building and testing aren't working (as noted in the comment).

Previously Nix had its own special build, but with the changes
to tket's CMake this is no longer necessary. As such, Nix now
uses the CMake build path, which has only had to change in one
manner: ./build had to be renamed to ./cmake_build, so as not
to remove python files previously placed there by setup.py.
Previously, the build directory being CWD/build caused problems in the
pytket setup.py process in a Nix build. Changing this to CWD/cmake_build
fixed the issue temporarily. Now I am defaulting back to CWD/build, but
allowing another directory to be provided with a BUILD_DIR environment
variable. This means the setup should be, for all intents and purposes,
identical on non-nix builds.
Previously tests were compiled into executables, but not registered
with ctest using catch's catch_discover_tests. I've added that in,
and added enable_testing to tket's CMakeLists.txt in the presence of
test flags.

This also allows for a simplification of the nix derivation - TKET
and its tests are now built as one project, then checked.
…ng tests in the test directory to allow for relative paths
@cqc-alec
Copy link
Collaborator

Probably need to add it to pytket's CMakeLists.txt as well?

@jake-arkinstall jake-arkinstall force-pushed the 1482-investigate-potential-cmake-improvements branch from 06b6e51 to 8598141 Compare August 13, 2024 14:35
@jake-arkinstall
Copy link
Contributor Author

jake-arkinstall commented Aug 13, 2024

I get linker errors, seemingly related to gmp (which I have installed).
...
Again this seems gmp-related.

Interesting. I think this goes back to SymEngine choosing not to expose its own dependencies, so it has to be added in. It doesn't appear that this is always necessary - CI didn't have an issue, and neither did the nix build. I've done so on Tket directly so pytket doesn't need to add it as was the case previously.

I've rebased from origin/main to resolve the conflict. I'd appreciate it if you could give it another go.

@cqc-alec
Copy link
Collaborator

I'd appreciate it if you could give it another go.

Adding gmp as a direct dependency of tket has fixed the first issue, but I still get the same runtime issue with pytket.

@jake-arkinstall
Copy link
Contributor Author

I'll spawn a docker build and see what's going on!

Copy link

This pull request has been automatically marked as stale.

@github-actions github-actions bot added the stale label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate potential CMake improvements
3 participants