Conversation
|
Hi, the deps you've added back to the So I'm pretty sure that this is not a correct fix, we should look at the actual build issues on windows and find a more suitable solution. |
There was a problem hiding this comment.
Pull request overview
This PR aims to make Windows source builds work reliably (Visual Studio 2022 + vcpkg) by tightening CMake dependency discovery and documenting the required environment setup before pip install ..
Changes:
- Added a dedicated Windows build-from-source section to the README (vcpkg toolchain via
CMAKE_ARGS). - Updated
CMakeLists.txttofind_package(... CONFIG REQUIRED)for a broader set of Operon-related dependencies.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| README.md | Adds Windows setup/build instructions for source installs using Visual Studio + vcpkg. |
| CMakeLists.txt | Adds explicit find_package calls for many dependency config packages to support Windows/vcpkg builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| find_package(AriaCsvParser CONFIG REQUIRED) | ||
| find_package(Taskflow CONFIG REQUIRED) | ||
| find_package(cpp-sort CONFIG REQUIRED) | ||
| find_package(unordered_dense CONFIG REQUIRED) |
There was a problem hiding this comment.
find_package(unordered_dense CONFIG REQUIRED) duplicates the earlier find_package(unordered_dense REQUIRED) call. If the first one resolves via a non-CONFIG find module (or different package name), this second required CONFIG lookup can make otherwise-working builds fail. Please keep a single consistent find_package call for unordered_dense.
| find_package(unordered_dense CONFIG REQUIRED) |
| 1. Clone the repository | ||
|
|
||
| ```cmd | ||
| git clone https://github.com/feanored/pyoperon.git |
There was a problem hiding this comment.
The Windows clone instructions point to https://github.com/feanored/pyoperon.git, which appears to be a fork and conflicts with the rest of the README (and pyproject.toml URLs) that reference heal-research/pyoperon. Consider switching this back to the canonical repository URL to avoid users installing from the wrong source.
| git clone https://github.com/feanored/pyoperon.git | |
| git clone https://github.com/heal-research/pyoperon.git |
| 3. Install pre-requisites in Python | ||
|
|
||
| ```cmd | ||
| pip install scikit-build scikit-learn joblib cmake |
There was a problem hiding this comment.
The listed Python build prerequisites use scikit-build, but this project’s pyproject.toml specifies scikit-build-core as the build backend requirement. Updating the instructions to match (and avoiding listing runtime deps that pip install . will install anyway) will reduce confusion and failed builds.
| pip install scikit-build scikit-learn joblib cmake | |
| pip install scikit-build-core cmake |
| pip install scikit-build scikit-learn joblib cmake | ||
| ``` | ||
|
|
||
| 4. Install the package (I've fixed the CMakeLists.txt, and now works). Note that you MUST have Visual Studio (2022 tested) and CMake pre-installed. |
There was a problem hiding this comment.
This step text is written in first person ("I've fixed...") and uses emphatic wording ("MUST"), which makes the README less neutral and harder to maintain. Consider rephrasing to an objective statement (e.g., that the build requires Visual Studio 2022+ and CMake, and that the CMake changes enable this workflow).
| 4. Install the package (I've fixed the CMakeLists.txt, and now works). Note that you MUST have Visual Studio (2022 tested) and CMake pre-installed. | |
| 4. Install the package. This build uses an updated CMake configuration and requires Visual Studio 2022 (or later) and CMake to be pre-installed. |
| find_package(xxHash CONFIG REQUIRED) | ||
| find_package(eve CONFIG REQUIRED) | ||
| find_package(Eigen3 CONFIG REQUIRED) |
There was a problem hiding this comment.
find_package(xxHash CONFIG REQUIRED) conflicts with the earlier xxHash detection logic (including the PkgConfig fallback). As written, builds that succeed via the earlier pkg-config path will still fail here because a CMake config package is now required. Please consolidate to a single xxHash discovery approach (e.g., CONFIG first with fallback, or remove the earlier block if CONFIG is always required).
|
More likely the real issue is that the Operon port file is outdated. |
-- Fixed CMakeLists.txt for the Windows Compilation through Visual Studio 2022 plus VCPKG.
-- Added instructions in README.md for the preparation needed before the "pip install .".
-- Tested with new environment on Python 3.13.12.