Conversation
markovskipetar
requested review from
IvoDD,
alexowens90 and
poodlewars
as code owners
|
Label error. Requires exactly 1 of: patch, minor, major. Found: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
PLACEHOLDER_BODY |
ArcticDB Code Review Summary
This PR replaces a monolithic 1200+ line API & Compatibility
Memory & Safety
Correctness
Code Quality
Testing
Build & Dependencies
Security
PR Title & Description
Documentation
Summary: Both previously-flagged blocking issues are resolved in the latest push: |
| name: Compile (${{inputs.platform}}) | ||
| if: | | ||
| always() && | ||
| !cancelled() |
There was a problem hiding this comment.
Why are there different files for _unix and _windows?
My main problem with this is the big amount of duplication between the two, so in the future when we change something we might forget to do it for the other workflow as well.
E.g. see this if: always() && !cancelled. It is missing on the windows build. Is there a reason for this?
I appreciate for some things we reqire completely different step (e.g. for Install Azurite). We can have Install Azurite (Unix) step and Install Azurite (Windows) step and skip them depending on the OS. E.g. see how we do this in build_steps.yml for reference.
However looking at the diff between the two the differences are not big:
E.g. windows doesn't build C++ tests. We can just pass run_cpp_tests=false or use if: matrix.os != "windows"
There are many others but we can go together through the diff and decide what can be easily unified.
| if: | | ||
| always() && | ||
| !cancelled() && | ||
| !failure() |
There was a problem hiding this comment.
This is another place where we differ between unix and windows? Any particular reason?
Also I think always() && !cancelled() && !failures() is equivalent to the default success()? Might be better to leave it to the default in this case.
| npm install -g azurite | ||
| npm_config_prefix=$(npm config get prefix) | ||
| npm_bin_dir="$npm_config_prefix" | ||
| if [[ "$RUNNER_OS" == "Windows" ]]; then |
There was a problem hiding this comment.
Do we need this if? This code should only be executed on windows.
| post-cleanup: 'none' | ||
|
|
||
| - name: Install ArcticDB from artifacts | ||
| shell: cmd /C call {0} |
There was a problem hiding this comment.
Out of curiosity why have we decided to use a non bash shell? I think this makes integrating the windonws and unix variants harder.
| else | ||
| cd python | ||
| python -m pytest --timeout=3600 -v --tb=line -n logical --dist worksteal \ | ||
| -m "not lmdb" \ |
There was a problem hiding this comment.
Why do we skip lmdb tests on windows?
3 participants
This PR introduces GitHub Actions workflows for building and testing ArcticDB using conda environments across multiple platforms:
build_with_conda.yml — Parent workflow triggered on push to master, PRs, and manual dispatch. Orchestrates builds for Linux x64, Linux ARM64, macOS ARM64, and Windows x64.
compile_and_test_with_conda_unix.yml — Reusable workflow for Unix platforms (Linux/macOS). Handles compilation, C++ tests, and Python tests (unit, integration, hypothesis, stress, compat, enduser).
compile_and_test_with_conda_win.yml — Reusable workflow for Windows. Same structure as Unix but with Windows-specific shell handling and build configuration.