Conversation
|
Label error. Requires exactly 1 of: patch, minor, major. Found: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| cd cpp/out/${{inputs.preset}}-build/ | ||
| ctest --output-on-failure | ||
| env: | ||
| ARCTICDB_USING_CONDA: 1 No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file. This should be fixed — most linters and editors expect a trailing newline in text files.
| ARCTICDB_USING_CONDA: 1 | |
| env: | |
| ARCTICDB_USING_CONDA: 1 |
| steps: | ||
| - uses: actions/checkout@v6.0.1 | ||
|
|
||
| - name: Configure sccache |
There was a problem hiding this comment.
The shell: bash -l {0} default shell is set for all steps, but the Windows caller (win_64 with preset: windows-cl-conda-release) requires cmd /C call {0} for steps that interact with the MSVC-activated conda environment. The original cpp_tests_win_64 job used shell: cmd /C call {0} for the Configure, Build, and Run steps specifically, and also cleared CMAKE_GENERATOR_PLATFORM and CMAKE_GENERATOR_TOOLSET before calling cmake.
With the reusable workflow using a single bash -l default, the Windows job will fail at the CMake configure and build steps because the MSVC environment (set by micromamba's conda activation) is not compatible with a bash shell in the same way. The enabled: false guard currently prevents this from running, but it should be documented or fixed before enabled is set to true for Windows.
Consider either:
- Adding a
shellinput parameter so callers can override per-platform, or - Documenting clearly that this reusable workflow is not yet suitable for Windows and should remain
enabled: falseuntil those differences are handled.
| post-cleanup: 'none' | ||
|
|
||
| - name: Configure C++ Tests | ||
| run: | |
There was a problem hiding this comment.
The init-shell is hardcoded to bash for all platforms. The original Windows job used init-shell: bash cmd.exe to also initialise the cmd shell. Since this workflow is called for Windows (win_64) with enabled: false, this will be a problem once enabled. The init-shell value may need to be a parameterised input (e.g. init_shell: string) defaulting to bash, with the Windows caller passing bash cmd.exe.
| if: | | ||
| always() && | ||
| !cancelled() && | ||
| inputs.enabled |
There was a problem hiding this comment.
The if condition uses inputs.enabled (a boolean), but for workflow_call triggered reusable workflows, inputs.enabled evaluates as a string in some contexts. Using inputs.enabled == true is more explicit and safe:
| inputs.enabled | |
| if: | | |
| always() && | |
| !cancelled() && | |
| inputs.enabled == true |
| # PYTHON_Conda_Tests ( TO DO !!! ) | ||
| python_tests_linux_64: | ||
| name: Python Tests (linux_64) - ${{matrix.type}} | ||
| if: | |
There was a problem hiding this comment.
The cpp_tests_osx_arm64 call omits the free_disk_space input, so it defaults to false. That is consistent with the original job (which also ran the free-disk-space step). However, the original macOS job did include the free-disk-space step. Please verify this omission is intentional — if macOS runners are also space-constrained, free_disk_space: true should be passed here.
| ctest --output-on-failure | ||
| env: | ||
| ARCTICDB_USING_CONDA: 1 | ||
| uses: ./.github/workflows/build_with_conda_cpp_tests.yml |
There was a problem hiding this comment.
The Windows job is silently disabled via enabled: false. Consider adding a comment explaining why (the reusable workflow uses bash -l as default shell and lacks the cmd-shell steps and CMAKE_GENERATOR_PLATFORM/CMAKE_GENERATOR_TOOLSET overrides required for MSVC builds on Windows). This makes it easier for future contributors to know what must be resolved before enabling.
|
test body |
| shell: cmd /C call {0} | ||
| run: | | ||
| set CMAKE_GENERATOR_PLATFORM= | ||
| set CMAKE_GENERATOR_TOOLSET= |
There was a problem hiding this comment.
Critical bug: environment variables set via set in a separate step do not persist to subsequent steps in GitHub Actions.
set CMAKE_GENERATOR_PLATFORM= and set CMAKE_GENERATOR_TOOLSET= only affect the shell for the duration of this step. The next step ("Build ArcticDB with conda") will still see the conda-activated CMAKE_GENERATOR_PLATFORM/CMAKE_GENERATOR_TOOLSET values, causing the MSVC Ninja build to fail exactly as it did before.
To fix this, clear the variables within the same step that runs pip install:
| set CMAKE_GENERATOR_TOOLSET= | |
| set CMAKE_GENERATOR_PLATFORM= | |
| set CMAKE_GENERATOR_TOOLSET= |
And remove this separate step — instead merge the set commands into the top of the "Build ArcticDB with conda (Windows)" step. Or use $env:CMAKE_GENERATOR_PLATFORM = '' via PowerShell and write them to $GITHUB_ENV.
| free_disk_space: true | ||
| install_mongodb: true | ||
| hypothesis_profile: ci_linux | ||
| run_cpp_tests: ${{inputs.run_cpp_tests || true}} |
There was a problem hiding this comment.
Bug: inputs.run_cpp_tests || true prevents users from ever disabling C++ tests.
In GitHub Actions expression language, false || true evaluates to true. So if a user triggers workflow_dispatch and sets run_cpp_tests: false, the expression ${{inputs.run_cpp_tests || true}} evaluates to true, and C++ tests will always run regardless.
The same applies to all four platform calls. Use the original guard logic or pass inputs.run_cpp_tests directly:
| run_cpp_tests: ${{inputs.run_cpp_tests || true}} | |
| run_cpp_tests: ${{inputs.run_cpp_tests}} |
The reusable workflow already defaults run_cpp_tests to true, so omitting the || true still runs tests on push/pull_request events (where inputs is empty and the default kicks in).
| env: | ||
| ARCTICDB_USING_CONDA: 1 | ||
| COMMANDLINE: ${{inputs.run_commandline}} | ||
| CI_MONGO_HOST: ${{inputs.install_mongodb && 'mongodb' || ''}} |
There was a problem hiding this comment.
Bug: CI_MONGO_HOST: mongodb will not resolve without a Docker service container.
The original python_tests_linux_64 and python_tests_linux_aarch64 jobs used a Docker service container:
services:
mongodb:
image: mongo:4.4This container made MongoDB reachable at the hostname mongodb (Docker's DNS). The new workflow removes the service container entirely and instead installs the MongoDB binary via the install_mongodb action or a manual curl. However, the install_mongodb action only copies the mongod binary to /usr/local/bin — it does not start a mongod daemon, and the hostname mongodb will not resolve without Docker networking.
As a result, tests requiring MongoDB will fail to connect. Options:
- Add a step to start
mongodlocally and setCI_MONGO_HOST: localhost. - Restore the service container via a
services:block (not directly possible in reusableworkflow_callworkflows without workarounds). - Start
mongodin the background as part of the install step and updateCI_MONGO_HOSTaccordingly.
| HYPOTHESIS_PROFILE: ${{inputs.hypothesis_profile}} | ||
| ARCTICDB_PYTEST_ARGS: ${{inputs.run_custom_pytest_command}} | ||
| STORAGE_TYPE: ${{inputs.persistent_storage == 'no' && 'LMDB' || inputs.persistent_storage}} | ||
| NODE_OPTIONS: --openssl-legacy-provider |
There was a problem hiding this comment.
Missing newline at end of file.
| NODE_OPTIONS: --openssl-legacy-provider | |
| NODE_OPTIONS: --openssl-legacy-provider |
| ARCTICDB_PYTEST_ARGS: ${{ inputs.run_custom_pytest_command }} | ||
| NODE_OPTIONS: --openssl-legacy-provider | ||
| linux_64: | ||
| uses: ./.github/workflows/conda_build_and_test.yml |
There was a problem hiding this comment.
Critical bug: wrong filename — all 4 jobs will fail.
The reusable workflow is called conda_build_and_test.yml here, but the file added in this PR is .github/workflows/build_with_conda_and_test.yml. GitHub will error with "workflow not found" for every job.
| uses: ./.github/workflows/conda_build_and_test.yml | |
| uses: ./.github/workflows/build_with_conda_and_test.yml |
| ) | ||
|
|
||
| - name: Archive build artifacts | ||
| uses: actions/upload-artifact |
There was a problem hiding this comment.
Critical bug: incomplete actions/upload-artifact step — compile artifacts are never uploaded.
The step is missing the @version pin, with.name, and with.path. Without these, the step is not valid YAML-schema-wise and GitHub Actions will reject it. The downstream Download build artifacts steps depend on the artifact name build-${{inputs.platform}}, so this needs to match.
| uses: actions/upload-artifact | |
| - name: Archive build artifacts | |
| uses: actions/upload-artifact@v4 | |
| if: always() | |
| with: | |
| name: build-${{inputs.platform}} | |
| retention-days: 7 | |
| path: | | |
| cpp/out/${{inputs.preset}}-build/ | |
| python/arcticdb_ext* | |
| python/**/*.so | |
| python/**/*.pyd |
| timeout_minutes: 10 | ||
| max_attempts: 3 | ||
| command: npm install -g azurite | ||
|
|
There was a problem hiding this comment.
Bug: broken sed regex — Windows drive-letter path conversion silently fails.
s|^$[A-Z]$:|/\1| does not capture the drive letter. $[A-Z] matches a literal $ followed by an uppercase letter, and \1 refers to a non-existent capture group. The original code in the monolithic workflow used \([A-Z]\) (BRE capture group):
| npm_bin_dir=$(echo "$npm_bin_dir" | sed 's|^\([A-Z]\):|/\1|' | tr '[:upper:]' '[:lower:]' | sed 's|\\|/|g') |
| install_mongodb: {required: false, type: boolean, default: false, description: Install MongoDB for tests. Linux only} | ||
| run_cpp_tests: {required: false, type: boolean, default: true, description: Whether to run C++ tests} | ||
| cpp_tests_enabled: {required: false, type: boolean, default: true, description: Whether C++ tests are supported on this platform} | ||
| skip_lmdb_tests: {required: false, type: boolean, default: false, description: Skip LMDB tests. Windows only} |
There was a problem hiding this comment.
The skip_lmdb_tests input is declared here but is never actually read anywhere in the workflow. The Windows pytest step at line 410 unconditionally applies -m "not lmdb" regardless of this input's value, so the input has no effect. Either wire it up to conditionally apply the marker expression, or remove the unused input.
| skip_lmdb_tests: {required: false, type: boolean, default: false, description: Skip LMDB tests. Windows only} | |
| skip_lmdb_tests: {required: false, type: boolean, default: false, description: Skip LMDB tests. Windows only} |
The pytest step should use:
-m "${{inputs.skip_lmdb_tests && 'not lmdb' || ''}}" \
| # ==================== JOB 3: PYTHON TESTS ==================== | ||
| python_tests: | ||
| name: Python Tests (${{inputs.platform}}) - ${{matrix.type}} | ||
| needs: [compile] |
There was a problem hiding this comment.
The python_tests job uses if: always() && !cancelled() which means it will proceed even when the upstream compile job failed (status failure). This wastes runner time attempting to download artifacts that were never uploaded.
The compile job itself uses if: always() && !cancelled(), so a failure there won't be cancelled — it will be failure. The condition should guard against a failed compile:
| needs: [compile] | |
| if: | | |
| always() && | |
| !cancelled() && | |
| needs.compile.result == 'success' |
| cache-environment-key: ${{inputs.cache_key}} | ||
| post-cleanup: 'none' | ||
|
|
||
| - name: Clear CMAKE_GENERATOR variables (Windows) |
There was a problem hiding this comment.
Dead step — environment variable changes in cmd /C call {0} do not persist across steps.
set CMAKE_GENERATOR_PLATFORM= and set CMAKE_GENERATOR_TOOLSET= only clear the variables for the duration of this step's shell process. The variables are re-read from the conda-activated environment when the next step starts a new process. This step does nothing useful and can be removed. The actual clearing is (correctly) done again inside the "Build ArcticDB with conda (Windows)" step immediately below.
| npm_config_prefix=$(npm config get prefix) | ||
| npm_bin_dir="$npm_config_prefix" | ||
| if [[ "$RUNNER_OS" == "Windows" ]]; then | ||
| npm_bin_dir=$(echo "$npm_bin_dir" | sed 's|^$[A-Z]$:|/\1|' | tr '[:upper:]' '[:lower:]' | sed 's|\\|/|g') |
There was a problem hiding this comment.
Bug: the sed pattern s|^$[A-Z]$:|/\1| is malformed and silently fails to convert the Windows drive-letter path.
$[A-Z] matches a literal $ followed by an uppercase letter — it is not a regex group and \1 refers to a non-existent capture group. On Windows, npm config get prefix returns something like C:\Users\...\AppData\Roaming\npm, so the path is never converted to a Unix-style format. The npm_bin_dir ends up as the original Windows path (lowercased but with backslashes replaced), which is not a valid path in MSYS2/Git Bash, causing echo "$npm_bin_dir" >> $GITHUB_PATH to add a useless entry.
The original code in the monolithic workflow used a correct pattern. Replace with:
| npm_bin_dir=$(echo "$npm_bin_dir" | sed 's|^$[A-Z]$:|/\1|' | tr '[:upper:]' '[:lower:]' | sed 's|\\|/|g') | |
| npm_bin_dir=$(echo "$npm_bin_dir" | sed 's|^\([A-Z]\):|/\1|' | tr '[:upper:]' '[:lower:]' | sed 's|\\|/|g') |
| env: | ||
| ARCTICDB_USING_CONDA: 1 | ||
| COMMANDLINE: ${{inputs.run_commandline}} | ||
| CI_MONGO_HOST: ${{inputs.install_mongodb && 'mongodb' || ''}} |
There was a problem hiding this comment.
Bug: CI_MONGO_HOST: mongodb will not resolve — no Docker service container is defined.
The original python_tests_linux_64 and python_tests_linux_aarch64 jobs in the monolithic workflow defined a services.mongodb container that made the hostname mongodb resolvable. This reusable workflow has no services: block, so the hostname mongodb does not exist. Any test that connects to $CI_MONGO_HOST will fail to connect.
MongoDB is installed as a process via ./.github/actions/install_mongodb (or manual download), but that listens on localhost, not mongodb. The env var should be:
| CI_MONGO_HOST: ${{inputs.install_mongodb && 'mongodb' || ''}} | |
| CI_MONGO_HOST: ${{inputs.install_mongodb && 'localhost' || ''}} |
| ARCTICDB_PYTEST_ARGS: ${{ inputs.run_custom_pytest_command }} | ||
| NODE_OPTIONS: --openssl-legacy-provider | ||
| linux_64: | ||
| uses: ./.github/workflows/conda_build_and_test.yml |
There was a problem hiding this comment.
Critical: wrong workflow filename — all 4 platform jobs will fail with "workflow not found".
The file added in this PR is .github/workflows/build_with_conda_and_test.yml, but every caller job here references ./.github/workflows/conda_build_and_test.yml. GitHub Actions resolves reusable workflows by exact filename, so this mismatch will cause all 4 jobs (linux_64, linux_aarch64, osx_arm64, win_64) to fail immediately.
| uses: ./.github/workflows/conda_build_and_test.yml | |
| uses: ./.github/workflows/build_with_conda_and_test.yml |
| free_disk_space: true | ||
| install_mongodb: true | ||
| hypothesis_profile: ci_linux | ||
| run_cpp_tests: ${{inputs.run_cpp_tests || true}} |
There was a problem hiding this comment.
Bug: inputs.run_cpp_tests || true always evaluates to true, making it impossible to disable C++ tests via workflow_dispatch.
In GitHub Actions expression language, false || true evaluates to true. So when a user triggers workflow_dispatch and explicitly sets run_cpp_tests: false, the expression ${{inputs.run_cpp_tests || true}} evaluates to true and C++ tests always run regardless of user intent.
The same issue exists on lines 56, 73, 90 for the other three platform jobs.
Use the input directly (it defaults to true in the reusable workflow already):
| run_cpp_tests: ${{inputs.run_cpp_tests || true}} | |
| run_cpp_tests: ${{inputs.run_cpp_tests}} |
ArcticDB Code Review SummaryPR #2980 — Refactor conda build for C++ tests, replacing the monolithic API & Compatibility
Memory & Safety
Correctness
Code Quality
Testing
Build & Dependencies
Security
PR Title & Description
|
| runner: ubuntu-22.04-arm | ||
| platform: linux_aarch64 | ||
| preset: linux-conda-release | ||
| cache_key: conda-env-linux_aarch64 |
There was a problem hiding this comment.
it looks like this cache_key can be constructed by with a simple conda-env-${{platform}}, can we remove it?
| run: | ||
| shell: bash -l {0} | ||
| steps: | ||
| - name: Debug inputs |
There was a problem hiding this comment.
You shouldn't need this step, these params can be seen in the job iteself
| on: | ||
| workflow_call: | ||
| inputs: | ||
| runner: {required: true, type: string, description: GitHub runner to execute on} |
There was a problem hiding this comment.
The description here is wrong, this parameter is used to specify the OS.
It might be better to also rename it to os
| with: | ||
| runner: windows-latest | ||
| platform: win_64 | ||
| preset: windows-cl-conda-release |
There was a problem hiding this comment.
Critical bug: cache_key is a required input in compile_and_test_with_conda_win.yml but is never passed here.
compile_and_test_with_conda_win.yml declares:
cache_key: {required: true, type: string, description: Unique conda environment cache key per platform}This call site omits cache_key, which will cause GitHub Actions to fail workflow validation with: "Required input 'cache_key' not provided". Add:
| preset: windows-cl-conda-release | |
| runner: windows-latest | |
| platform: win_64 | |
| preset: windows-cl-conda-release | |
| cache_key: conda-env-win_64 | |
| run_cpp_tests: ${{inputs.disable_cpp_tests != true}} |
| runner: {required: true, type: string, description: GitHub runner to execute on} | ||
| platform: {required: true, type: string, description: Platform identifier e.g. linux_64 or osx_arm64} | ||
| preset: {required: true, type: string, description: CMake preset name e.g. linux-conda-release} | ||
|
|
There was a problem hiding this comment.
cpp_tests_enabled input is declared but never set to false by any caller, making it a no-op guard.
No call site in build_with_conda.yml passes cpp_tests_enabled: false. The default is true, so the inputs.cpp_tests_enabled condition in the cpp_tests job if: block (line 93) is always true and never suppresses anything. Either:
- Remove
cpp_tests_enabledand rely solely onrun_cpp_tests, or - Document which platforms should pass
cpp_tests_enabled: false(e.g., Windows) and wire it up at the call sites.
This is a different input from run_cpp_tests — the distinction between "user requested to disable" vs "platform doesn't support it" is valid, but callers need to actually use it.
|
|
||
| - name: Install ArcticDB from artifacts | ||
| run: | | ||
| if [ -d "cpp/out/${{inputs.preset}}-build" ] && (ls python/arcticdb_ext*.so 2>/dev/null | head -1 | grep -q .); then |
There was a problem hiding this comment.
Artifact detection for macOS may fail silently if *.so glob matches nothing.
The check ls python/arcticdb_ext*.so 2>/dev/null | head -1 | grep -q . relies on the .so extension. On macOS conda builds the extension is .so (not .dylib), so this should work. However, if the artifact download placed files in a subdirectory rather than directly under python/, this check will fail and the ARCTIC_CMAKE_PRESET=skip optimisation will be bypassed, causing a full (slow) rebuild. Consider adding a fallback:
| if [ -d "cpp/out/${{inputs.preset}}-build" ] && (ls python/arcticdb_ext*.so 2>/dev/null | head -1 | grep -q .); then | |
| if [ -d "cpp/out/${{inputs.preset}}-build" ] && (ls python/arcticdb_ext*.so python/arcticdb_ext*.pyd 2>/dev/null | head -1 | grep -q .); then |
| runner: ubuntu-22.04-arm | ||
| platform: linux_aarch64 | ||
| preset: linux-conda-release | ||
| run_cpp_tests: ${{inputs.disable_cpp_tests != true}} |
There was a problem hiding this comment.
linux_aarch64 does not pass hypothesis_profile, so the unix workflow falls back to the dev profile (via inputs.hypothesis_profile || 'dev' at unix workflow line 301). The original monolithic workflow used HYPOTHESIS_PROFILE: ci_linux for aarch64. The dev profile runs fewer hypothesis examples, making these tests less thorough than before.
Consider adding hypothesis_profile: ci_linux to match the linux_64 call.
| run_cpp_tests: ${{inputs.disable_cpp_tests != true}} | |
| run_cpp_tests: ${{inputs.disable_cpp_tests != true}} |
markovskipetar
requested review from
IvoDD,
alexowens90 and
poodlewars
as code owners
build.txt
Outdated
| @@ -0,0 +1,871 @@ | |||
| Using pip 24.2 from /turbo/pmarkovski/pyenvs/ArcticDb_VirtEnv/lib/python3.11/site-packages/pip (python 3.11) | |||
There was a problem hiding this comment.
Accidental commit: local build log should not be in the repository.
This file contains output from a local pip install run on a developer's machine (paths like /turbo/pmarkovski/) and has no place in the repository. It adds noise, leaks internal path information, and will conflict with others' work.
Please remove build.txt before merging (add it to .gitignore if needed to prevent future accidents).
| uses: SimenB/github-actions-cpu-cores@v2.0.0 | ||
| id: cpu-cores | ||
|
|
||
| - name: Print cache key |
There was a problem hiding this comment.
Debug step should be removed before merge.
This step echoes the cache key on every CI run and adds noise to logs. The cache key (conda-env-${{ inputs.platform }}) is already visible in the setup-micromamba step output and in the workflow inputs. G-D-Petrov flagged the same pattern on the earlier "Debug inputs" step (#39).
| - name: Print cache key |
| @@ -0,0 +1 @@ | |||
| import arcticdb No newline at end of file | |||
There was a problem hiding this comment.
I think that this file can be deleted
| # ==================== JOB 1: COMPILE ==================== | ||
| compile: | ||
| name: Compile (${{inputs.platform}}) | ||
| if: | |
There was a problem hiding this comment.
I don't think that you need this if
markovskipetar
changed the title
| ARCTICDB_USING_CONDA: 1 | ||
| COMMANDLINE: ${{inputs.run_commandline}} | ||
| ARCTICDB_PYTEST_ARGS: ${{inputs.run_custom_pytest_command}} | ||
| NODE_OPTIONS: --openssl-legacy-provider No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file.
| NODE_OPTIONS: --openssl-legacy-provider | |
| NODE_OPTIONS: --openssl-legacy-provider |
3 participants
Reference Issues/PRs
What does this implement or fix?
Any other comments?
Checklist
Checklist for code changes...