Add nix support #918
Merged
Add nix support #918
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n GCC 12.2 tket/src/Circuit/CircUtils.cpp lines 1230 and 1237 clearly only deference the std::optional within a checked branch. However, GCC 12.2 gives: ``` /build/tket/src/Circuit/CircUtils.cpp: In function 'std::tuple<tket::Circuit, std::array<SymEngine::Expression, 3>, tket::Circuit> tket::normalise_TK2_angles(Expr, Expr, Expr)': /build/tket/src/Circuit/CircUtils.cpp:1237:13: error: '*(double*)((char*)&b_eval + offsetof(std::optional<double>,std::optional<double>::<unnamed>.std::_Optional_base<double, true, true>::<unnamed>))' may be used uninitialized [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmaybe-uninitialized-Werror=maybe-uninitialized8;;] 1237 | *b_eval -= 3; | ~~~~~~~~^~~~ /build/tket/src/Circuit/CircUtils.cpp:1148:25: note: '*(double*)((char*)&b_eval + offsetof(std::optional<double>,std::optional<double>::<unnamed>.std::_Optional_base<double, true, true>::<unnamed>))' was declared here 1148 | std::optional<double> b_eval = eval_expr_mod(b, 4); | ^~~~~~ /build/tket/src/Circuit/CircUtils.cpp:1230:13: error: '*(double*)((char*)&a_eval + offsetof(std::optional<double>,std::optional<double>::<unnamed>.std::_Optional_base<double, true, true>::<unnamed>))' may be used uninitialized [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmaybe-uninitialized-Werror=maybe-uninitialized8;;] 1230 | *a_eval -= 3; | ~~~~~~~~^~~~ /build/tket/src/Circuit/CircUtils.cpp:1147:25: note: '*(double*)((char*)&a_eval + offsetof(std::optional<double>,std::optional<double>::<unnamed>.std::_Optional_base<double, true, true>::<unnamed>))' was declared here 1147 | std::optional<double> a_eval = eval_expr_mod(a, 4); ``` An alternative fix is to apply a `#pragma GCC diagnostic ignore "-Wmaybe-uninitialized"` to the regions in question.
…not fall through. For the modified cases, the switch statements would fall through if no return path was hit. Although the assert is fired when no return is performed anyway, this isn't clear to the compiler, and a fallthrough warning (thus failure due to -Werror) occurs. Adding these breaks makes the intent clear to the compiler and avoids such issues.
This isn't conceptually ideal, but is harmless for this case. Global size_t is referred to in these files and I didn't want to make too many changes here. As per the standard, size_t imported from the <c...> headers only guarantee its presence in the std namespace, not the global namespace, though implementations may take liberties.
Some key points to note about this implementation. First, the imported source files avoid default.nix itself such that changes to default.nix will not be considered changes to the project source, and thus won't trigger unnecessary rebuilds. Second, Nix+CMake awkwardly double-prepended the nix store path to the include directory, rendering it e.g.: /nix/store/[hash]-tklog//nix/store/[hash]-tklog To avoid this, a post phase is added to strip this unwanted behaviour, allowing it to be imported from other projects without failure.
Refer to d3ae2c96 (Added default.nix for tklog., 2023-07-03) for implementation details.
See d3ae2c96 (Added default.nix for tklog., 2023-07-03) for implementation details.
See d3ae2c96 (Added default.nix for tklog., 2023-07-03) for implementation details.
See d3ae2c96 (Added default.nix for tklog., 2023-07-03) for implementation details.
This exports an attrset of name->package for each package within libs.
pkgs.symengine doesn't propagate its dependencies, so those are injected explicitly via pkgs.symengine.buildInputs. The same implementation details as the implementation in d3ae2c96 (Added default.nix for tklog., 2023-07-03) apply.
The concept is somewhat different to the original intent of setup.py, simply due to keeping Nix in control of the build. Nix, rather than setup.py, compiles binders/. It then passes that - along with python package dependencies such as numpy etc - to the setup process. It also passes a flag: USE_NIX=1 The setup process now detects USE_NIX and chooses a NixBuild build_ext to put the pieces together, rather than the previous options of CMake or Conan. In the absense of USE_NIX, the setup process is unchanged. Whether or not "doing nothing" is the appropriate form of NixBuild, I do not yet know. Probably not. But until we get to that stage, we need to solve another issue: `types-pkg_resources` is a listed dependency, and we aren't providing it. It isn't something that's in nixpkgs, and I'm struggling to find any useful information about it online.
…, and removed the one I intended to remove.
Added nix support for types-pkg_resources via fetchPypi, and established build of pytket. At current, tests cause permission errors so are disabled. This must be remedied in future, or otherwise provided in a separate derivation such that tests can be run. At current, no confirmation that the build is successful and that libraries are available to pytket when running. In particular, at the moment I'm making .a files, rather than .so files, available for tket and libs, which may or may not work. If it doesn't work, an alternative build will need to be added.
…red libraries on request, and provided the shared alternatives to pytket.
- Overloaded symengine (static and shared), such that build inputs are propagated - Patch libtket.so so that symengine is listed as a needed dependency - Symlink rather than copy libtket.so to pytket's _tket dir so that fixupPhase doesn't remove symengine - Manually setting pytket.__version__ to 0.0.0, as scm-based versioning is somewhat messy with Nix - Other small structural changes This needs a lot of cleanup before any PR would be possible, but as pytket can be imported for the first time with this Nix approach, it's worthy of a commit.
…xed point combinators instead of exposing derivations directly
…, and adding a workflow to run nix flake check
|
Looks good, I think this just needs:
|
|
I've now added python tests, so
I'll look into that now.
I've now added this. It points to a README.md in the nix-support directory. I've also added an example usage flake within there, and documented that (lightly) to provide an example of what simple usage might look like. |
|
Test matrix now added. This will be the first time this approach has been tried on MacOS. I'll run it on my macbook first and diagnose any issues that may arise, then once that's done I'm happy to promote this from a draft PR. |
…o symengine, not to the mythical LIB library.
…n -DINSTALL_NAME_DIR=OFF is passed (which the Nix build now does). Added -flat_namespace link arg to pytket binders to avoid symbol confusion when using pybind11 (specifically, typecast.hpp's dynamic casts failed, though the types were 'correct' and reinterpret_casts worked). Cleaned up the nix section of setup.py as libtket is automagically included. Updated flake.nix to take advantage of an upstream change to symengine which makes shared library usage easier to deal with.
…and the cmake builds within libs/
|
All tests passing on MacOS now. It was surprisingly awkward due to dynamic_casts failing, and it seems to be thanks to dylib's two-level namespaces (identified thanks to https://developer.apple.com/forums/thread/693914). I want to verify my changes on Linux again, do some tidying around the edges, then I'll promote this to a full PR. |
cqc-alec
reviewed
Oct 20, 2023
| src = tket-without-tests; | ||
| nativeBuildInputs = [ super.cmake ]; | ||
| propagatedBuildInputs = super.tklibs | ||
| ++ [ super.boost super.symengine super.eigen super.nlohmann_json ]; |
There was a problem hiding this comment.
Do we specify the versions of these libraries anywhere or does nix just pick up the latest available?
There was a problem hiding this comment.
The versions of the packages are defined by the nixpkgs commit that the flake.lock file is pinned to.
The typical way of bumping the packages when they're available in a newer version of nixpkgs is to run "nix flake update", which updates the lock file.
If a version is needed that isn't in nixpkgs yet, or if the version in nixpkgs is too new, or if something needs to be tweaked, the derivation of the dependency can be defined in a nix file within tket (this is what I did with jsonschema and SymEngine, for example).
cqc-alec
reviewed
Oct 20, 2023
| src = super.fetchFromGitHub { | ||
| owner = "CQCL"; | ||
| repo = "qwasm"; | ||
| rev = "35ebe1e2551449d97b9948a600f8d2e4d7474df6"; |
There was a problem hiding this comment.
There's likely to be a certain amount of bitrot here as dependency versions get updated; maybe we could prevent this with some ugly CI checks but probably not worth it for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not yet ready for review. Aside from possible design changes, documentation and test automation need to be added too. Testing on MacOS is also required. Windows support is not generally available in Nix at this stage (to my knowledge).
Nix is an alternative, hermetic way of managing packages. With this PR, I've added support for building within Nix.
The
nix.flakefile specifies the core nix dependencies. It also specifies where the build instructions can be found - these are bundled into a newnix-supportdirectory.The structure of
nix-supportavoids having nix files scattered throughout the project. The package files are:symengine.nix: provides a derivation of SymEngine built to suit the needs of this project. Specifically, throughsymengine.patch, dependencies are linked as required. Additionally, symengine.nix provides both static and shared library versions of SymEngine.third-party-python-packages.nix- provides packages missing from nixpkgs. These are:libs.nix- provides the following as static and shared derivations:tket.nix- provides tket, as both static and shared derivationspytket.nix- provides the binders and pytket, using only the shared derivations.An additional change is to
setup.py, which now has a nix-specific build extension. This uses the environment variables provided by nix to find tklog, tket, and binders. CMakeLists.txt for tket is now also told not to error on -wmaybe-uninitialized, because of a false positive that crops up in GCC 12.2. If this isn't wanted for general builds, this flag can be added as a patch for nix-specific builds.These derivations are all provided through fixed point combinators, so that flake.nix can use them as overloads to pkgs.
Additional changes that are required for compilation, though not specific enough for this PR, are:
std::size_tis provided, not C'ssize_t. Most implementations accept this, but GCC 12.2 does not.tket/include/tket/ArchAwareSynth/SteinerTree.hpp. Although an assert is fired on default paths, indirectly blocking fallthrough, the compiler can't always detect this, and warns on possible fallthrough. This causes a failed build due to-Werror.Example usage (replace the github organisation once merged, and this requires the above additional changes to run) is below. Note that the machine does not need any dependencies, including python or GCC, installed in advance. It takes some time to compile - and gives my threadripper a run for its money, but promises a reliable build.