-
Notifications
You must be signed in to change notification settings - Fork 47
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
base: main
Are you sure you want to change the base?
Cmake improvements and Nix unification with existing pytket setup #1501
Conversation
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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
4913ad0
to
9abb317
Compare
Thanks Jake, looks good. When testing locally I encountered a couple of issues (following my usual local workflow). First, when building tket with 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:
Again this seems gmp-related. |
There was a problem hiding this 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
Probably need to add it to pytket's |
06b6e51
to
8598141
Compare
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. |
Adding gmp as a direct dependency of tket has fixed the first issue, but I still get the same runtime issue with pytket. |
I'll spawn a docker build and see what's going on! |
This pull request has been automatically marked as stale. |
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):
rmtree
command was also removing python setup outputs within a nix build, as those happened to also be placed insidebuild/
. 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.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