Add Windows build support with clang-cl and vcpkg#450
Open
lorisercole wants to merge 37 commits intomainfrom
Open
Add Windows build support with clang-cl and vcpkg#450lorisercole wants to merge 37 commits intomainfrom
lorisercole wants to merge 37 commits intomainfrom
Conversation
Two issues caused 27 test failures on Windows: 1. NATIVE_ULONG / size_t mismatch: On Windows LLP64, unsigned long is 4 bytes while size_t is 8 bytes. Use NATIVE_UINT64 instead, which is unambiguously 64-bit on all platforms. 2. NATIVE_HBOOL / bool mismatch: HDF5's hbool_t is unsigned int (4 bytes), but C++ bool is 1 byte. Reading/writing a bool* with NATIVE_HBOOL is undefined behavior. Use hbool_t intermediaries with static_cast at each serialization site.
On Windows, std::filesystem::remove() fails if the file still has an open handle — unlike Linux, which allows unlinking open files. Two tests opened an std::ifstream to verify FCIDUMP file contents but called remove() while the stream was still in scope, causing: "remove: The process cannot access the file because it is being used by another process" Fix by scoping each ifstream in a block so its destructor closes the file handle before the remove() call. Affected tests: - HamiltonianTest.SparseContainerFCIDUMP - HamiltonianTest.FCIDUMPActiveSpaceConsistency
Shell::from_json() used default initialization (`Shell sh`), leaving
the rpowers array indeterminate for non-ECP shells — the JSON round-
trip never writes rpowers for regular shells. Both BasisHasher and
BasisEqChecker read rpowers unconditionally, so comparing a database-
loaded shell (zero-initialized via `Shell sh{0}`) against a JSON-
deserialized shell was undefined behavior. Fix by value-initializing
(`Shell sh{}`), matching the pattern already used elsewhere.
Also fix BasisSetMap test which had three bugs:
- Reversed basis_json["shells"] but the actual key is "electron_shells"
(operator[] silently created an empty entry, making the reverse a no-op)
- Used Hydrogen (1 shell in STO-3G), so even reversing the correct key
would be a no-op; switch to Lithium which has 3 shells
- Test only passed on Linux by accident: uninitialized rpowers garbage
happened to make the hashes differ
Three issues prevented the Python package from building and importing on Windows when linking to a pre-installed C++ library: 1. pybind11 not found: vcpkg's find_package wrapper intercepts the search and misses pybind11 installed in pip's isolated build environment. Fix by querying `python -m pybind11 --cmakedir` and setting pybind11_DIR explicitly before find_package. 2. DLL load failure at import: Python 3.8+ no longer searches PATH for DLL dependencies of extension modules. Add a Windows-only block in __init__.py that reads the QDK_DLL_DIR environment variable and calls os.add_dll_directory() for each path before _core is imported. 3. Test script updates: use Ninja generator, clang-cl compilers, vcpkg toolchain, CMAKE_PREFIX_PATH for the pre-built C++ library, uv for venv management, and QDK_DLL_DIR for runtime DLL resolution.
In Windows the default encoding is cp1252, not UTF-8. This caused tests to fail.
NamedTemporaryFile with the default delete=True keeps an exclusive file handle on Windows, preventing C++ code from opening the same path. Use delete=False so the handle is released when the with-block exits, then clean up manually with Path.unlink() in a finally block. This pattern is needed wherever a temp file is created in Python and then passed to the C++ library for reading or writing. Affected tests: test_basis_set, test_orbitals, test_stability, test_noise_models (24 instances total). Note: when the minimum Python version is raised to 3.12+, this can be simplified using delete_on_close=False, which releases the file handle on close but auto-deletes on context manager exit — removing the need for manual unlink() calls.
The check_license_headers.py script prints a ✅ emoji on success, but Python defaults to cp1252 encoding for stdout on Windows, which cannot encode it. Reconfigure stdout to UTF-8 at the top of the script.
On Windows, spdlog::stdout_color_mt() creates a wincolor_stdout_sink that caches a Windows HANDLE via GetStdHandle(STD_OUTPUT_HANDLE) at construction time. All subsequent writes go through WriteFile(HANDLE) which bypasses the C runtime's file descriptor layer entirely. When pytest's capfd redirects fd 1 via dup2(), the cached HANDLE still points to the original stdout, so all Logger output bypasses capture — causing ~25 test failures across test_logger.py, test_constants_documentation.py, and test_scf.py. On Linux this is not a problem because stdout_color_sink_mt is aliased to ansicolor_stdout_sink, which writes via fwrite(stdout) through the C runtime's fd layer. Add a custom stdout_fd_sink for Windows that writes via fwrite(stdout), giving Windows the same fd-based write path that Linux gets by default. The sink is used only on Windows (#ifdef _WIN32); Linux/macOS continue to use stdout_color_mt() unchanged.
Windows uses cp1252 as the default locale encoding, which silently corrupts non-ASCII text in open(), read_text(), write_text(), and subprocess.run(text=True) calls. All text I/O in the package and its test suite now passes encoding="utf-8" explicitly, and subprocess invocations additionally propagate PYTHONIOENCODING=utf-8 to child processes so that both parent and child agree on the codec regardless of the system locale. A dedicated round-trip regression test verifies that Unicode content (φ, Å, ΔE, αβγ, 你好) survives file serialization and stdout capture end-to-end on Windows.
The uv-installed CPython 3.14 on Windows ships with an incomplete Tcl/Tk installation (missing tcl_findLibrary). Matplotlib defaults to the TkAgg backend, which crashes when trying to create a Tk window during circuit diagram plotting tests. Force the non-interactive Agg backend in conftest.py before any pyplot import. This is standard practice for CI/headless test environments and avoids the Tk dependency entirely.
This eliminates CRLF warnings on Windows and ensures consistent line endings across platforms.
Run each example script in a fresh TemporaryDirectory so output files (e.g. .h5, .json) don't pollute the source tree. On failure the temp directory is preserved for debugging; on success it is cleaned up.
Using more than 2 OpenMP threads causes some tests to fail (SCF not converging, ...) due to numerical instabilities. See: https://gist.github.com/lorisercole/c8081e5f4966c6bf9a2e64734c781fd3 The culprit appears to be GauXC's XC integrator, that uses element-by-element #pragma omp atomic accumulation on shared matrices (inc_by_submat_atomic in util.hpp). Under LLVM's libomp this causes non-deterministic floating-point summation order, leading to NaN/divergence in SCF with >2 threads. The issue does not manifest with GCC/libgomp due to its more conservative thread scheduling. An issue on GauXC repo will be opened. Temporary fix: disable GAUXC_ENABLE_OPENMP on MSVC while keeping OpenMP enabled for the rest of the project (MACIS, our own code).
lorisercole
requested review from
ConradJohnston,
nabbelbabbel,
sobolevnrm,
v-georgen and
wavefunction91
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the first slice of Windows-native build support for QDK/Chemistry, targeting clang-cl + MSVC Build Tools and vcpkg-managed native dependencies, while hardening runtime/test behavior for Windows-specific IO and encoding pitfalls.
Changes:
- Add vcpkg manifests + an OpenBLAS overlay port (incl. LAPACK/C_LAPACK and cross-build getarch handling) to provide Windows-native dependency builds.
- Update CMake and third-party build wiring for MSVC/clang-cl compatibility (flags, OpenMP handling, dependency warning suppression, Fortran mangling override).
- Improve Windows portability across Python + C++: UTF-8 file/stdio handling, DLL search path registration, HDF5 LLP64 fixes, and test updates for Windows file locking.
Reviewed changes
Copilot reviewed 56 out of 57 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Adds repo vcpkg manifest for Windows dependency resolution. |
| vcpkg-configuration.json | Pins vcpkg baseline and enables overlay ports. |
| vcpkg-overlay/ports/openblas/vcpkg.json | Defines overlay OpenBLAS port manifest for Windows/LAPACK needs. |
| vcpkg-overlay/ports/openblas/portfile.cmake | Builds OpenBLAS with embedded C LAPACK and cross-build getarch handling. |
| vcpkg-overlay/ports/openblas/cmake-project-include.cmake | Forces sensible TARGET/NUM_THREADS defaults for vcpkg scenarios. |
| vcpkg-overlay/ports/openblas/disable-testing.diff | Disables OpenBLAS tests under BUILD_TESTING control. |
| vcpkg-overlay/ports/openblas/getarch.diff | Allows using host-built getarch binaries and installs them when possible. |
| vcpkg-overlay/ports/openblas/system-check-msvc.diff | Improves MSVC processor detection in OpenBLAS system checks. |
| vcpkg-overlay/ports/openblas/win32-uwp.diff | Uses WIN32 checks instead of string system-name comparisons. |
| vcpkg-overlay/ports/openblas/openblas_common.h | Adds OpenBLAS ABI/config helpers for Windows builds. |
| python/CMakeLists.txt | Improves pybind11 discovery under vcpkg and adjusts coverage/RPATH logic for Windows. |
| python/pyproject.toml | Updates dev extras and scikit-build-core configuration verbosity key. |
| python/src/qdk_chemistry/init.py | Adds Windows DLL directory registration + UTF-8 stdout/stderr configuration. |
| python/src/qdk_chemistry/data/base.py | Forces UTF-8 for JSON file IO. |
| python/src/qdk_chemistry/data/noise_models.py | Forces UTF-8 for YAML file IO. |
| python/src/qdk_chemistry/utils/qsharp/init.py | Reads Q# sources using UTF-8 explicitly. |
| python/src/qdk_chemistry/utils/cubegen.py | Reads generated cube files using UTF-8. |
| python/tests/conftest.py | Ensures UTF-8 test output and forces non-interactive matplotlib backend. |
| python/tests/test_basis_set.py | Avoids Windows file locking by closing temp files and unlinking manually. |
| python/tests/test_circuit.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_docs_examples.py | Uses UTF-8 reads and runs examples in temp dirs with UTF-8 subprocess env. |
| python/tests/test_energy_estimator.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_energy_expectation_result.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_estimator_data.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_mc.py | Skips PySCF-dependent tests when unavailable; updates SCF solver comment. |
| python/tests/test_measurement_data.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_noise_models.py | Fixes Windows temp-file locking and adds UTF-8 round-trip/subprocess test. |
| python/tests/test_orbitals.py | Avoids Windows file locking for JSON/HDF5 temp files; manual unlink cleanup. |
| python/tests/test_qubit_hamiltonian.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_readme_snippets.py | Forces UTF-8 subprocess decoding and log file writing. |
| python/tests/test_sample_workflow.py | Skips notebook tests requiring PySCF when not installed. |
| python/tests/test_sample_workflow_utils.py | Forces UTF-8 decoding and subprocess IO encoding env. |
| python/tests/test_settings.py | Reads JSON test outputs using UTF-8. |
| python/tests/test_stability.py | Avoids Windows temp-file locking for JSON/HDF5; manual unlink cleanup. |
| cpp/CMakeLists.txt | Adds MSVC/clang-cl oriented flags, warning suppression, OpenMP notes, and BLAS mangling define. |
| cpp/cmake/qdk-uarch.cmake | Maps uarch flags for MSVC and improves compiler-aware handling. |
| cpp/cmake/third_party.cmake | Removes -fPIC for MSVC builds; disables OpenMP in GauXC on Windows; updates GauXC pin. |
| cpp/include/qdk/chemistry/utils/stdout_fd_sink.hpp | Adds Windows sink to make spdlog stdout compatible with pytest capfd/dup2. |
| cpp/src/qdk/chemistry/utils/logger.cpp | Uses stdout_fd_sink on Windows to enable stdout capture. |
| cpp/src/qdk/chemistry/data/hdf5_serialization.cpp | Fixes LLP64 portability by storing size_t vectors as NATIVE_UINT64. |
| cpp/src/qdk/chemistry/data/orbitals.cpp | Writes/reads HDF5 boolean fields via hbool_t intermediaries. |
| cpp/src/qdk/chemistry/data/wavefunction_containers/cc.cpp | Writes HDF5 boolean attribute via hbool_t intermediate. |
| cpp/tests/CMakeLists.txt | Limits coverage flags to GCC/Clang toolchains. |
| cpp/tests/test_hamiltonian.cpp | Ensures file handles close before delete to avoid Windows file locking errors. |
| cpp/src/qdk/chemistry/algorithms/microsoft/scf/src/core/basis_set.cpp | Improves filesystem stem string conversion and zero-initializes Shell. |
| cpp/src/qdk/chemistry/algorithms/microsoft/scf/tests/util_tests.cpp | Adjusts basis shell ordering test to use lithium + correct JSON key. |
| external/macis/include/macis/types.hpp | Adds MSVC-specific includes and a MSVC-only uint128 placeholder type. |
| external/macis/include/macis/bitset_operations.hpp | Adds MSVC implementations for CLZ/FFS and adapts uint128 conversion. |
| external/macis/cmake/macis-ips4o.cmake | Avoids linking atomic on Windows. |
| external/macis/cmake/macis-spdlog.cmake | Avoids -fPIC on MSVC. |
| external/macis/src/lobpcgxx/CMakeLists.txt | Avoids -fPIC on MSVC; forces BLAS Fortran mangling define on Windows. |
| external/macis/python/tests/test_pymacis.py | Forces UTF-8 when reading/writing FCIDUMP test files. |
| .pipelines/pip-scripts/windows-build-clang-cmake.ps1 | Adds Windows local build script using clang-cl + vcpkg + CMake. |
| .pipelines/pip-scripts/windows-build-clang-pip.ps1 | Adds Windows local build script using clang-cl + vcpkg + pip/scikit-build-core. |
| .github/scripts/check_license_headers.py | Forces UTF-8 stdout to avoid Windows console encoding issues. |
| .gitattributes | Enforces LF line endings repo-wide. |
| .gitignore | Ignores local Windows build artifacts (install/, vcpkg_installed/). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Coverage Summary
Detailed Coverage ReportsC++ Coverage DetailsPython Coverage DetailsPybind11 Coverage Details |
`os.add_dll_directory` only exists on Windows – mypy on Linux reports it as missing.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
On Windows, importing qdk_chemistry required manually setting QDK_DLL_DIR to the vcpkg bin directory, unlike Linux/macOS where it just works via RPATH. Fix this by: - Copying vcpkg runtime DLLs (openblas, hdf5, fmt, etc.) into the wheel alongside _core.pyd at build time (python/CMakeLists.txt) - Auto-registering the package directory as a DLL search path in __init__.py (required since Python 3.8 changed DLL search behavior) - Removing QDK_DLL_DIR from build scripts (no longer needed) QDK_DLL_DIR is kept as an optional override for custom setups.
…ruct These operators are only used by macis tests.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+207
to
+218
| # On Windows, bundle vcpkg runtime DLLs alongside _core.pyd so the package | ||
| # is self-contained and does not require QDK_DLL_DIR at import time. | ||
| if(WIN32 AND VCPKG_INSTALLED_DIR AND VCPKG_TARGET_TRIPLET) | ||
| file(GLOB _vcpkg_runtime_dlls | ||
| "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/bin/*.dll" | ||
| ) | ||
| if(_vcpkg_runtime_dlls) | ||
| install(FILES ${_vcpkg_runtime_dlls} | ||
| DESTINATION . | ||
| COMPONENT _core | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
The install step bundles vcpkg bin/*.dll, but if OpenMP is enabled with clang-cl the runtime dependency is typically libomp.dll from the VS LLVM toolchain (not vcpkg). This means the installed _core.pyd may still fail to import on machines without VS/LLVM on PATH, despite the comment claiming the package is self-contained. Consider either bundling the OpenMP runtime DLL as well when OpenMP is found on Windows, or disabling OpenMP by default for the Python wheel/install component.
Suggested change
| # On Windows, bundle vcpkg runtime DLLs alongside _core.pyd so the package | |
| # is self-contained and does not require QDK_DLL_DIR at import time. | |
| if(WIN32 AND VCPKG_INSTALLED_DIR AND VCPKG_TARGET_TRIPLET) | |
| file(GLOB _vcpkg_runtime_dlls | |
| "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/bin/*.dll" | |
| ) | |
| if(_vcpkg_runtime_dlls) | |
| install(FILES ${_vcpkg_runtime_dlls} | |
| DESTINATION . | |
| COMPONENT _core | |
| ) | |
| endif() | |
| # On Windows, bundle runtime DLLs alongside _core.pyd so the package | |
| # is self-contained and does not require QDK_DLL_DIR or a compiler toolchain | |
| # runtime on PATH at import time. | |
| if(WIN32) | |
| if(VCPKG_INSTALLED_DIR AND VCPKG_TARGET_TRIPLET) | |
| file(GLOB _vcpkg_runtime_dlls | |
| "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/bin/*.dll" | |
| ) | |
| if(_vcpkg_runtime_dlls) | |
| install(FILES ${_vcpkg_runtime_dlls} | |
| DESTINATION . | |
| COMPONENT _core | |
| ) | |
| endif() | |
| endif() | |
| # clang-cl/OpenMP commonly depends on libomp.dll from the LLVM toolchain | |
| # rather than from vcpkg. Bundle it as well when OpenMP is enabled. | |
| if(OpenMP_CXX_FOUND) | |
| set(_openmp_runtime_dll_candidates) | |
| foreach(_openmp_library IN LISTS OpenMP_CXX_LIBRARIES) | |
| if(EXISTS "${_openmp_library}") | |
| get_filename_component(_openmp_lib_dir "${_openmp_library}" DIRECTORY) | |
| get_filename_component(_openmp_lib_parent "${_openmp_lib_dir}" DIRECTORY) | |
| list(APPEND _openmp_runtime_dll_candidates | |
| "${_openmp_lib_dir}/libomp.dll" | |
| "${_openmp_lib_dir}/bin/libomp.dll" | |
| "${_openmp_lib_parent}/bin/libomp.dll" | |
| ) | |
| endif() | |
| endforeach() | |
| if(CMAKE_CXX_COMPILER) | |
| get_filename_component(_cxx_compiler_dir "${CMAKE_CXX_COMPILER}" DIRECTORY) | |
| get_filename_component(_cxx_compiler_parent "${_cxx_compiler_dir}" DIRECTORY) | |
| list(APPEND _openmp_runtime_dll_candidates | |
| "${_cxx_compiler_dir}/libomp.dll" | |
| "${_cxx_compiler_dir}/bin/libomp.dll" | |
| "${_cxx_compiler_parent}/bin/libomp.dll" | |
| ) | |
| endif() | |
| set(_openmp_runtime_dlls) | |
| foreach(_openmp_runtime_dll IN LISTS _openmp_runtime_dll_candidates) | |
| if(EXISTS "${_openmp_runtime_dll}") | |
| list(APPEND _openmp_runtime_dlls "${_openmp_runtime_dll}") | |
| endif() | |
| endforeach() | |
| if(_openmp_runtime_dlls) | |
| list(REMOVE_DUPLICATES _openmp_runtime_dlls) | |
| install(FILES ${_openmp_runtime_dlls} | |
| DESTINATION . | |
| COMPONENT _core | |
| ) | |
| endif() | |
| endif() |
Contributor
Author
There was a problem hiding this comment.
The vcpkg install directory is (should be) project-scoped (not a system-wide dir), so it only contains DLLs from declared dependencies.
Here are the currently bundled DLLs:
aec.dll
fmt.dll
hdf5.dll
hdf5_cpp.dll
openblas.dll
spdlog.dll
szip.dll
zlib1.dll
and here is the dependency tree:
_core.pyd
├── fmt.dll (vcpkg, bundled)
├── hdf5_cpp.dll (vcpkg, bundled)
│ └── hdf5.dll (vcpkg, bundled)
│ ├── zlib1.dll (vcpkg, bundled)
│ └── szip.dll (vcpkg, bundled)
├── openblas.dll (vcpkg, bundled)
├── spdlog.dll (vcpkg, bundled)
│ └── fmt.dll (vcpkg, bundled)
├── libomp140.x86_64.dll (LLVM toolchain, NOT bundled ⚠️)
└── system DLLs (always present)
Only aec.dll is not used.
libomp140.x86_64.dll is indeed not bundled. We should decide what to do with it...
PR #434 introduced breaking changes to the `QuantumErrorProfile` API, so we need to adapt one of `test_noise_models` tests.
lorisercole
force-pushed
the
feature/ler/windows-build-part-1
branch
from
96c6d4b to
03638ca
Compare
Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (4)
vcpkg-overlay/ports/openblas/vcpkg.json:1
- This overlay port manifest declares a dependency on
openblas(itself) as a host dependency, which creates a circular dependency and will fail resolution in vcpkg. Remove the self-dependency; if host tools are required, they should be provided via a separate*-hostport or handled within the portfile usingVCPKG_HOST_*variables rather than a manifest self-reference.
python/tests/test_docs_examples.py:1 - Managing
TemporaryDirectorymanually here makes cleanup depend on the success path; if an exception is raised before the return code check (e.g., timeout), the directory may leak. Use a context manager (with TemporaryDirectory(...) as tmpdir:) or atry/finallyto ensure cleanup happens deterministically (and only keep artifacts explicitly when desired).
python/tests/test_docs_examples.py:1 - Managing
TemporaryDirectorymanually here makes cleanup depend on the success path; if an exception is raised before the return code check (e.g., timeout), the directory may leak. Use a context manager (with TemporaryDirectory(...) as tmpdir:) or atry/finallyto ensure cleanup happens deterministically (and only keep artifacts explicitly when desired).
vcpkg-overlay/ports/openblas/cmake-project-include.cmake:1 - Correct spelling of 'explixit' to 'explicit'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+249
to
250
| with Path(yaml_file).open("r", encoding="utf-8") as f: | ||
| data = yaml.load(f) |
There was a problem hiding this comment.
Using yaml.load without an explicit loader can deserialize arbitrary Python objects, which is unsafe when reading untrusted YAML. Use yaml.safe_load (or yaml.load(..., Loader=...) with a restricted loader) to prevent object construction vulnerabilities.
Comment on lines
+190
to
+195
| // Use NATIVE_UINT64 (not NATIVE_ULONG) because on Windows LLP64 | ||
| // unsigned long is 4 bytes while size_t is 8 bytes. | ||
| static_assert(sizeof(size_t) == 8); | ||
| H5::DataSet dataset = group.createDataSet( | ||
| dataset_name, H5::PredType::NATIVE_ULONG, dataspace); | ||
| dataset.write(vector.data(), H5::PredType::NATIVE_ULONG); | ||
| dataset_name, H5::PredType::NATIVE_UINT64, dataspace); | ||
| dataset.write(vector.data(), H5::PredType::NATIVE_UINT64); |
There was a problem hiding this comment.
The unconditional static_assert(sizeof(size_t) == 8) makes the code fail to compile on any 32-bit target (where size_t is 4 bytes). If 32-bit builds are not supported, consider documenting/enforcing that at a higher level; otherwise, select the HDF5 integer type based on sizeof(size_t) (e.g., NATIVE_UINT32 vs NATIVE_UINT64) to keep serialization portable across 32/64-bit platforms.
Comment on lines
+231
to
+232
| static_assert(sizeof(size_t) == 8); | ||
| dataset.read(vector.data(), H5::PredType::NATIVE_UINT64); |
There was a problem hiding this comment.
The unconditional static_assert(sizeof(size_t) == 8) makes the code fail to compile on any 32-bit target (where size_t is 4 bytes). If 32-bit builds are not supported, consider documenting/enforcing that at a higher level; otherwise, select the HDF5 integer type based on sizeof(size_t) (e.g., NATIVE_UINT32 vs NATIVE_UINT64) to keep serialization portable across 32/64-bit platforms.
Suggested change
| static_assert(sizeof(size_t) == 8); | |
| dataset.read(vector.data(), H5::PredType::NATIVE_UINT64); | |
| static_assert(sizeof(size_t) == 4 || sizeof(size_t) == 8, | |
| "Unsupported size_t width for HDF5 deserialization."); | |
| const H5::PredType& size_type = sizeof(size_t) == 4 | |
| ? H5::PredType::NATIVE_UINT32 | |
| : H5::PredType::NATIVE_UINT64; | |
| dataset.read(vector.data(), size_type); |
Comment on lines
+209
to
+219
| if(WIN32 AND VCPKG_INSTALLED_DIR AND VCPKG_TARGET_TRIPLET) | ||
| file(GLOB _vcpkg_runtime_dlls | ||
| "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/bin/*.dll" | ||
| ) | ||
| if(_vcpkg_runtime_dlls) | ||
| install(FILES ${_vcpkg_runtime_dlls} | ||
| DESTINATION . | ||
| COMPONENT _core | ||
| ) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Installing every DLL from the vcpkg bin/ directory can significantly bloat the wheel and may introduce unexpected/unused binaries (and potential DLL collision/hijacking risks if unrelated DLLs get shipped). Prefer copying only the transitive runtime dependencies of _core (e.g., via CMake runtime dependency discovery like file(GET_RUNTIME_DEPENDENCIES ...) / install(RUNTIME_DEPENDENCIES ...)) and explicitly exclude system DLLs.
This file contains hidden or 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
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.
This is the first of a series of PRs to introduce Windows native support.
It enables building QDK/Chemistry on Windows using
clang-cl(via MSVC Build Tools) and vcpkg for native dependencies.Summary of changes:
_USE_MATH_DEFINES, Fortran name-mangling override (BLAS_FORTRAN_ADD_), suppressed overly-verbose LLVM warnings for dependencies.vcpkg.jsonmanifest and a custom OpenBLAS overlay port to build LAPACK on WindowsNATIVE_ULONGwithNATIVE_UINT64forsize_tvectors (Windowsunsigned longis 4 bytes), and usehbool_tintermediaries instead of writingbool*withNATIVE_HBOOLstdout_fd_sinkthat writes viafwrite(stdout)instead of caching a Windows HANDLE, so pytestcapfdworks afterdup2()redirectionQDK_DLL_DIRenv var andos.add_dll_directory(); enforce UTF-8 encoding for stdout/stderr and all file I/O.gitattributesenforcing LF line endings, fix uninitializedShell::rpowers, PowerShell build scripts, updated GauXC pin[For more detailed information, check the commit messages and comments in the code.]
To be addressed by future PRs:
clcompiler