-
Notifications
You must be signed in to change notification settings - Fork 58
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
C++ improvements #63
Merged
Merged
C++ improvements #63
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
674e914
Refactor cpp structure
markkohdev 9a68237
Start to add tests
markkohdev 059c7bd
Try to clean up my mess a bit. Also add PR template
markkohdev 024eee0
Clean up documentation and got tests running
markkohdev 074b1bb
Uncomment the other tests but they failing right now
markkohdev 5a1e371
Fix java and python cpp src paths
markkohdev d86ebbb
Improve formatting strategy
markkohdev 314bd82
Run C++ formatter
markkohdev 9cdbc4e
Add .clang-format to root for CI
markkohdev 9888690
Update clang-format action container
markkohdev 6a873c2
Fix find command
markkohdev 98214a5
Start to replace Catch2 with doctest. Dummy test only
markkohdev befa6b2
Remove commented test code until namespacing is implemented
markkohdev 6722eb6
Add some actual doctest tests
markkohdev f2f1156
Use doctest and add combination var checks
markkohdev ec40dc9
Add c++ tests to github workflows
markkohdev 152b894
Fix rebase error
markkohdev cf09244
Update javadoc
markkohdev 3f87ed0
Undo docs changes for cleaner PR
markkohdev ba06fd2
Add EOF EOL
markkohdev 0532250
Revert cpp spacing to 80 for cleaner PR
markkohdev 598e9e6
Fix TypedIndex rebase
markkohdev 753175f
Add c++ tests to github actions
markkohdev 4f98c93
Add build dir for cmake
markkohdev 1f6c0b1
Fix windows tests
markkohdev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
cpp/.clang-format | ||
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
## Description | ||
<!-- Describe the changes introduced by this pull request and why they are necessary. --> | ||
|
||
## Related Issues | ||
<!-- Reference any related GitHub issues that are addressed by this pull request. --> | ||
|
||
## Changes Made | ||
<!-- Provide a brief overview of the changes made in this pull request. --> | ||
|
||
### C++ | ||
<!-- Describe changes made to the C++ code, including any new features, improvements, or bug fixes. --> | ||
|
||
### Python | ||
<!-- Describe changes made to the Python code, including any new features, improvements, or bug fixes. --> | ||
|
||
### Java | ||
<!-- Describe changes made to the Java code, including any new features, improvements, or bug fixes. --> | ||
|
||
## Testing | ||
<!-- Outline the testing strategy employed to validate these changes. Include any relevant test cases or scenarios. --> | ||
|
||
## Checklist | ||
<!-- | ||
Replace [ ] with [x] to check off items. | ||
For items that are not applicable, simply remove the checkbox. | ||
--> | ||
|
||
- [ ] My code follows the code style of this project. | ||
- [ ] I have added and/or updated appropriate documentation (if applicable). | ||
- [ ] All new and existing tests pass locally with these changes. | ||
- [ ] I have run static code analysis (if available) and resolved any issues. | ||
- [ ] I have considered backward compatibility (if applicable). | ||
- [ ] I have confirmed that this PR does not introduce any security vulnerabilities. | ||
|
||
## Additional Comments | ||
<!-- Add any additional comments or notes that might be helpful for reviewers or contributors. --> |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,8 +39,38 @@ jobs: | |
- name: Check C++ Formatting | ||
uses: jidicula/[email protected] | ||
with: | ||
clang-format-version: 14 | ||
fallback-style: LLVM | ||
clang-format-version: 16 | ||
|
||
run-cpp-tests: | ||
runs-on: ${{ matrix.os }} | ||
continue-on-error: true | ||
defaults: | ||
run: | ||
working-directory: cpp | ||
strategy: | ||
matrix: | ||
# TODO: Switch back to macos-latest once https://github.com/actions/python-versions/pull/114 is fixed | ||
os: | ||
- 'ubuntu-latest' | ||
- windows-latest | ||
- macos-12 | ||
name: Test C++ on ${{ matrix.os }} | ||
steps: | ||
- uses: actions/checkout@v3 | ||
with: | ||
submodules: recursive | ||
- name: Install CMake (Windows) | ||
if: matrix.os == 'windows-latest' | ||
run: | | ||
choco install cmake --installargs 'ADD_CMAKE_TO_PATH=System' | ||
- name: Install CMake (MacOS) | ||
if: matrix.os == 'macos-12' | ||
run: brew install cmake | ||
- name: Install CMake (Ubuntu) | ||
if: matrix.os == 'ubuntu-latest' | ||
run: sudo apt-get install -y cmake | ||
- name: Run tests | ||
run: make test | ||
|
||
run-java-tests: | ||
continue-on-error: true | ||
|
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[submodule "cpp/include/doctest"] | ||
path = cpp/include/doctest | ||
url = [email protected]:doctest/doctest.git |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,91 +1,81 @@ | ||
# How to Contribute | ||
|
||
We'd love to get patches from you! | ||
|
||
## Getting Started | ||
#### Workflow | ||
We follow the [GitHub Flow Workflow](https://guides.github.com/introduction/flow/): | ||
|
||
### Prerequisites | ||
1. Fork the project | ||
1. Check out the `master` branch | ||
1. Create a feature branch | ||
1. Write code and tests for your change | ||
1. From your branch, make a pull request against `https://github.com/spotify/voyager` | ||
1. Work with repo maintainers to get your change reviewed | ||
1. Wait for your change to be pulled into `https://github.com/spotify/voyager/master` | ||
1. Delete your feature branch | ||
|
||
## Getting Started | ||
### Prerequisites | ||
To compile Voyager from scratch, the following packages will need to be installed: | ||
|
||
- [Python 3.7](https://www.python.org/downloads/) or higher. | ||
- A C++ compiler, e.g. `gcc`, `clang`, etc. | ||
|
||
### Building Voyager | ||
#### Building Python | ||
There are some nuances to building the Voyager python code. Please read on for more information. | ||
|
||
For basic building, you should be able to simply run the following commands: | ||
```shell | ||
git clone [email protected]:spotify/voyager.git | ||
cd voyager | ||
pip3 install -r python/dev-requirements.txt | ||
pip3 install . | ||
cd python | ||
pip install -r python/dev-requirements.txt | ||
pip install . | ||
``` | ||
|
||
To compile a debug build of `voyager` that allows using a debugger (like gdb or lldb), use the following command to build the package locally and install a symbolic link for debugging: | ||
```shell | ||
cd python | ||
DEBUG=1 python3 setup.py build develop | ||
DEBUG=1 python setup.py build develop | ||
``` | ||
|
||
Then, you can `import voyager` from Python (or run the tests with `tox`) to test out your local changes. | ||
|
||
> If you're on macOS or Linux, you can try to compile a debug build _faster_ by using [Ccache](https://ccache.dev/): | ||
> ## macOS | ||
> ```shell | ||
> brew install ccache | ||
> rm -rf build && CC="ccache clang" CXX="ccache clang++" DEBUG=1 python3 -j8 -m pip install -e . | ||
> rm -rf build && CC="ccache clang" CXX="ccache clang++" DEBUG=1 python -j8 -m pip install -e . | ||
> ``` | ||
> ## Linux | ||
> e.g. | ||
> ```shell | ||
> sudo yum install ccache # or apt, if on a Debian | ||
> | ||
> # If using GCC: | ||
> rm -rf build && CC="ccache gcc" CXX="scripts/ccache_g++" DEBUG=1 python3 setup.py build -j8 develop | ||
> rm -rf build && CC="ccache gcc" CXX="scripts/ccache_g++" DEBUG=1 python setup.py build -j8 develop | ||
> | ||
> # ...or if using Clang: | ||
> rm -rf build && CC="ccache clang" CXX="scripts/ccache_clang++" DEBUG=1 python3 setup.py build -j8 develop | ||
> rm -rf build && CC="ccache clang" CXX="scripts/ccache_clang++" DEBUG=1 python setup.py build -j8 develop | ||
> ``` | ||
|
||
### Updating Documentation | ||
If you notice that the documentation is out of date, feel free to run these commands in order to update the docs and make a PR with the changes. | ||
|
||
#### Python | ||
While `voyager` is mostly C++ code, it ships with `.pyi` files to allow for type hints in text editors and via MyPy. To update the Python type hint files, use the following commands: | ||
|
||
```shell | ||
cd python | ||
python3 -m scripts.generate_type_stubs_and_docs | ||
# Documentation will be dumped into ../docs/python/ | ||
``` | ||
|
||
#### Java | ||
To update the javadocs for the java bindings, you can simply run: | ||
|
||
#### Building Java | ||
To build the Java library with `maven`, use the following commands: | ||
```shell | ||
cd java | ||
mvn package | ||
``` | ||
|
||
this will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java). | ||
|
||
## Workflow | ||
|
||
We follow the [GitHub Flow Workflow](https://guides.github.com/introduction/flow/): | ||
|
||
1. Fork the project | ||
1. Check out the `master` branch | ||
1. Create a feature branch | ||
1. Write code and tests for your change | ||
1. From your branch, make a pull request against `https://github.com/spotify/voyager` | ||
1. Work with repo maintainers to get your change reviewed | ||
1. Wait for your change to be pulled into `https://github.com/spotify/voyager/master` | ||
1. Delete your feature branch | ||
#### Building C++ | ||
To build the C++ library with `cmake`, use the following commands: | ||
```shell | ||
cd cpp | ||
git submodule update --init --recursive | ||
make build | ||
``` | ||
|
||
## Testing | ||
|
||
### Python Tests | ||
We use `tox` for testing - running tests from end-to-end should be as simple as: | ||
|
||
``` | ||
cd python | ||
pip3 install tox | ||
tox | ||
``` | ||
|
@@ -110,10 +100,26 @@ asv continuous --sort name --no-only-changed HEAD main | |
Please note that `airspeed-velocity` can only run benchmarks against a git commit, so if | ||
you have uncommited code that you want to run benchmarks for you need to commit it first. | ||
|
||
## Style | ||
### Java Tests | ||
We provide java test execution as a maven test step. Thus you can run the tests with: | ||
|
||
```shell | ||
cd java | ||
mvn verify | ||
``` | ||
|
||
### C++ Tests | ||
To run the C++ tests, use the following commands: | ||
```shell | ||
cd cpp | ||
git submodule update --init --recursive | ||
make test | ||
``` | ||
|
||
## Style | ||
Use [`clang-format`](https://clang.llvm.org/docs/ClangFormat.html) for C++ code, and `black` with defaults for Python code. | ||
|
||
### Python | ||
In order to check and run formatting within the python module, you can use tox to facilitate this. | ||
```bash | ||
cd python | ||
|
@@ -123,8 +129,43 @@ tox -e check-formatting | |
tox -e format | ||
``` | ||
|
||
## Issues | ||
### C++ | ||
If you are working on any C++ code throughout the repo, ensure you have `clang-format` (version 16) installed, and then use clang-format to handle C++ formatting: | ||
```bash | ||
cd cpp | ||
cmake . | ||
# Check formatting only (don't change files) | ||
make check-formatting | ||
# Run formatter | ||
make format | ||
``` | ||
|
||
### Updating Documentation | ||
We also welcome improvements to the project documentation or to the existing | ||
docs. Please file an [issue](https://github.com/spotify/voyager/issues/new). | ||
|
||
If you notice that the generated API documentation is out of date, feel free to run these commands in order to update the docs and make a PR with the changes. | ||
|
||
#### Python | ||
While `voyager` is mostly C++ code, it ships with `.pyi` files to allow for type hints in text editors and via MyPy. To update the Python type hint files, use the following commands: | ||
|
||
```shell | ||
cd python | ||
python3 -m scripts.generate_type_stubs_and_docs | ||
# Documentation will be dumped into ../docs/python/ | ||
``` | ||
|
||
#### Java | ||
To update the javadocs for the java bindings, you can simply run: | ||
|
||
```shell | ||
cd java | ||
mvn package | ||
``` | ||
|
||
This will update the java documentation located in [docs/java/](https://github.com/spotify/voyager/tree/main/docs/java). | ||
|
||
## Issues | ||
When creating an issue please try to ahere to the following format: | ||
|
||
One line summary of the issue (less than 72 characters) | ||
|
@@ -141,47 +182,7 @@ When creating an issue please try to ahere to the following format: | |
|
||
List all relevant steps to reproduce the observed behaviour. | ||
|
||
## Pull Requests | ||
|
||
Files should be exempt of trailing spaces. | ||
|
||
We adhere to a specific format for commit messages. Please write your commit | ||
messages along these guidelines. Please keep the line width no greater than 80 | ||
columns (You can use `fmt -n -p -w 80` to accomplish this). | ||
|
||
One line description of your change (less than 72 characters) | ||
|
||
Problem | ||
|
||
Explain the context and why you're making that change. What is the problem | ||
you're trying to solve? In some cases there is not a problem and this can be | ||
thought of being the motivation for your change. | ||
|
||
Solution | ||
|
||
Describe the modifications you've done. | ||
|
||
Result | ||
|
||
What will change as a result of your pull request? Note that sometimes this | ||
section is unnecessary because it is self-explanatory based on the solution. | ||
|
||
Some important notes regarding the summary line: | ||
|
||
* Describe what was done; not the result | ||
* Use the active voice | ||
* Use the present tense | ||
* Capitalize properly | ||
* Do not end in a period — this is a title/subject | ||
* Prefix the subject with its scope | ||
|
||
## Documentation | ||
|
||
We also welcome improvements to the project documentation or to the existing | ||
docs. Please file an [issue](https://github.com/spotify/voyager/issues/new). | ||
|
||
## First Contributions | ||
|
||
If you are a first time contributor to `voyager`, familiarize yourself with the: | ||
* [Code of Conduct](CODE_OF_CONDUCT.md) | ||
* [GitHub Flow Workflow](https://guides.github.com/introduction/flow/) | ||
|
@@ -192,20 +193,15 @@ When you're ready, navigate to [issues](https://github.com/spotify/voyager/issue | |
There is a lot to learn when making your first contribution. As you gain experience, you will be able to make contributions faster. You can submit an issue using the [question](https://github.com/spotify/voyager/labels/question) label if you encounter challenges. | ||
|
||
# License | ||
|
||
By contributing your code, you agree to license your contribution under the | ||
terms of the [LICENSE](https://github.com/spotify/voyager/blob/master/LICENSE). | ||
|
||
# Code of Conduct | ||
|
||
Read our [Code of Conduct](CODE_OF_CONDUCT.md) for the project. | ||
|
||
# Troubleshooting | ||
|
||
## Building the project | ||
|
||
### `ModuleNotFoundError: No module named 'pybind11'` | ||
|
||
Try updating your version of `pip`: | ||
```shell | ||
pip install --upgrade pip | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
--- | ||
BasedOnStyle: LLVM | ||
IndentWidth: 2 | ||
InsertNewlineAtEOF: true | ||
--- | ||
Language: Cpp | ||
# Use 120 columns since we have big screens now | ||
ColumnLimit: 80 |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
softlink to allow the clang-format github action to work correctly