Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI workflow adjusted: Windows runner changed to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| env: | ||
| TEMP: ${{ matrix.platform == 'windows' && 'D:\a\_temp' || runner.temp }} | ||
| TMP: ${{ matrix.platform == 'windows' && 'D:\a\_temp' || runner.temp }} |
There was a problem hiding this comment.
I don't know how this works. What was this for?
There was a problem hiding this comment.
The standard windows-latest runner currently has a D: drive that is larger and faster that C:. This just configured env vars so that D: would be used for temp files for various build tools. The larger runner does not have a separate D: drive, apparently.
…olchain action; add more config options
3b04e5e to
77fee76
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci_test.yml (1)
213-214: Consider whether--profile testis the right choice for artifact reuse.The workflow uses
--profile testwith the goal of reusing build artifacts betweencargo buildand laterpytesttests (as noted in the merge conflict context). However,Cargo.tomldoes not define a custom[profile.test], so Cargo's built-intestprofile is used—which enables test harness compilation (test = true).While this works in practice (the codebase has no compile-time test gating), using the
devprofile or defining an explicit[profile.test]inCargo.tomlwould be clearer and more conventional. This would also align better with the stated goal of artifact reuse without relying on the built-in test profile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci_test.yml around lines 213 - 214, The workflow currently runs "maturin develop --profile test" which relies on Cargo's built-in test profile; instead either change the workflow to use "maturin develop --profile dev" so artifacts match "cargo build"/'dev' semantics, or explicitly add a [profile.test] section to Cargo.toml to define the test profile you intend to reuse; update the CI step that runs "maturin develop --profile test" (or add the Cargo.toml [profile.test] entry) and ensure the pytest invocation "pytest -s tests" continues to run against the produced artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci_test.yml:
- Around line 141-147: Resolve the Git merge conflict markers in the "Build oxen
rust project" step by removing the conflict lines (<<<<<<< HEAD, =======,
>>>>>>>) and keep the version that uses the test profile: ensure the run command
uses "cargo build --workspace --profile test" so the workflow remains valid YAML
and preserves the intended behavior of not rebuilding later; update the step's
run block (the "Build oxen rust project" run command) accordingly.
---
Nitpick comments:
In @.github/workflows/ci_test.yml:
- Around line 213-214: The workflow currently runs "maturin develop --profile
test" which relies on Cargo's built-in test profile; instead either change the
workflow to use "maturin develop --profile dev" so artifacts match "cargo
build"/'dev' semantics, or explicitly add a [profile.test] section to Cargo.toml
to define the test profile you intend to reuse; update the CI step that runs
"maturin develop --profile test" (or add the Cargo.toml [profile.test] entry)
and ensure the pytest invocation "pytest -s tests" continues to run against the
produced artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1aef1bd1-4aec-45c9-b719-7301e2a56710
📒 Files selected for processing (1)
.github/workflows/ci_test.yml
2054d1b to
239b10d
Compare
…verything use the dev profile instead
…patibility in order to build libduckdb-sys
…rent syntax on a windows vhd
D:drive like the free runner does, so stop using thatactions-rust-lang/setup-rust-toolchainaction was already callingSwatinem/rust-cacheunder-the-hood, and that was actually running, so make that explicit and set a couple flags to cache moreSwatinum/rust-cachestep wasn't actually getting used, so remove thatmain, but we were actually uploading on every build throughsetup-rust-toolchain(above).setup-rust-toolchaindoesn't plumb-through that option, but it wasn't using that option anyway...so it's no change.lldlinker that ships with Rust on Windows