refactor: rename ddlog_lille to lille_ddlog and update build configuration#96
refactor: rename ddlog_lille to lille_ddlog and update build configuration#96
Conversation
Reviewer's GuideThis PR systematically renames the generated DDlog crate from “ddlog_lille” to “lille_ddlog” across the build configuration, scripts, documentation, and CI pipeline, and removes the now-obsolete generated crate metadata. Class diagram for BuildOptions struct documentation updateclassDiagram
class BuildOptions {
+bool fail_on_ddlog_error
+Option<PathBuf> ddlog_dir // now refers to lille_ddlog instead of ddlog_lille
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update renames the generated DDlog crate from Changes
Sequence Diagram(s)sequenceDiagram
participant CI
participant Makefile
participant DDlog Compiler
participant Rust Project
CI->>Makefile: Run build-support-run (Build DDlog inferencer)
Makefile->>DDlog Compiler: Compile .dl file
DDlog Compiler->>Makefile: Output lille_ddlog crate
Makefile->>Rust Project: Integrate lille_ddlog crate
CI->>CI: Set environment variables (CS_ACCESS_TOKEN, CODESCENE_CLI_SHA256)
CI->>CI: Upload coverage if CS_ACCESS_TOKEN is set
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- In build_support/src/lib.rs the default ddlog_dir now unwraps to OUT_DIR directly—consider restoring the join("lille_ddlog") so the generated crate lands in OUT_DIR/lille_ddlog as intended.
- Double-check scripts/build_support_runner.sh and perform a global search for any remaining “ddlog_lille” references to ensure the rename is fully applied across all build scripts and configs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In build_support/src/lib.rs the default ddlog_dir now unwraps to OUT_DIR directly—consider restoring the join("lille_ddlog") so the generated crate lands in OUT_DIR/lille_ddlog as intended.
- Double-check scripts/build_support_runner.sh and perform a global search for any remaining “ddlog_lille” references to ensure the rename is fully applied across all build scripts and configs.
## Individual Comments
### Comment 1
<location> `build_support/src/lib.rs:33` </location>
<code_context>
#[ortho_config(default = false)]
pub fail_on_ddlog_error: bool,
- /// Destination directory for the generated `ddlog_lille` crate.
- /// If not provided, defaults to `OUT_DIR/ddlog_lille`.
+ /// Destination directory for the generated `lille_ddlog` crate.
+ /// If not provided, defaults to `OUT_DIR/lille_ddlog`.
pub ddlog_dir: Option<std::path::PathBuf>,
</code_context>
<issue_to_address>
The documentation update is correct, but the default value logic below is now inconsistent.
Update the code to set the default to `out_dir.join("lille_ddlog")` to match the documentation and avoid confusion about the output location.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #[ortho_config(default = false)] | ||
| pub fail_on_ddlog_error: bool, | ||
|
|
||
| /// Destination directory for the generated `ddlog_lille` crate. |
There was a problem hiding this comment.
issue: The documentation update is correct, but the default value logic below is now inconsistent.
Update the code to set the default to out_dir.join("lille_ddlog") to match the documentation and avoid confusion about the output location.
…ation - Rename ddlog_lille dependency to lille_ddlog in Cargo.toml - Update build support and CI configuration for new naming - Remove obsolete generated/ddlog_lille/Cargo.toml - Update documentation references to reflect new naming convention - Modify Makefile and build scripts to use new ddlog package name
leynos
force-pushed
the
build-ddlog-in-generated
branch
from
d327676 to
8f94e6e
Compare
- Update build-inferencer target to reference generated/lille_ddlog - Fix path references for DDlog build process
- Move secrets and vars to env section for proper access - Update conditional check to use env.CS_ACCESS_TOKEN - Fix CodeScene coverage upload configuration
- Add fallback empty string for CS_ACCESS_TOKEN secret to prevent workflow failures - Resolves IDE warning about invalid context access
- Move fix_static.patch from generated/ddlog_lille/patches/ to patches/ - Update Makefile build-inferencer target to reference new patch location - Clean up generated directory structure by removing empty patches subdirectory - Improves project organization by centralizing patches in root-level directory
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Makefile (1)
47-50: Manifest path still points to the old directory – build will break
cargo build --manifest-path generated/ddlog_lille/lille_ddlog/Cargo.tomlcontradicts the earlier rename togenerated/lille_ddlog. CI will fail once the old directory is deleted.- RUSTFLAGS="-D warnings" cargo build --manifest-path generated/ddlog_lille/lille_ddlog/Cargo.toml --target-dir targets/ddlog + RUSTFLAGS="-D warnings" cargo build --manifest-path generated/lille_ddlog/lille_ddlog/Cargo.toml --target-dir targets/ddlog
♻️ Duplicate comments (1)
build_support/src/lib.rs (1)
33-34: Fix documentation inconsistency with default value.The documentation states the default is
OUT_DIR/lille_ddlog, but the code on line 84 uses justOUT_DIRwithout the subdirectory. This creates confusion about the actual output location.Apply this diff to align the code with the documentation:
- .unwrap_or_else(|| out_dir.clone()); + .unwrap_or_else(|| out_dir.join("lille_ddlog"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
generated/ddlog_lille/Cargo.tomlis excluded by!**/generated/**
📒 Files selected for processing (8)
.github/workflows/ci.yml(2 hunks)Cargo.toml(1 hunks)Makefile(2 hunks)build_support/src/ddlog.rs(1 hunks)build_support/src/lib.rs(2 hunks)docs/architecture.md(1 hunks)docs/ddlog-migration-roadmap.md(1 hunks)docs/ddlog-world-inference.md(1 hunks)
🔇 Additional comments (8)
Cargo.toml (1)
18-19: LGTM! Consistent crate renaming.The dependency name, path, and feature reference have been correctly updated to maintain consistency with the new
lille_ddlognaming convention.Also applies to: 23-23
docs/architecture.md (1)
77-77: Documentation correctly updated to reflect crate renaming.The reference to the DDlog runtime library has been properly updated to match the new
lille_ddlogcrate name.docs/ddlog-world-inference.md (1)
266-267: Code example correctly updated for crate renaming.The import statement and accompanying comment have been properly updated to use the new
lille_ddlogcrate name, ensuring the documentation example remains functional.build_support/src/ddlog.rs (1)
20-20: Documentation parameter description correctly updated.The function documentation now accurately reflects the new
lille_ddlogcrate name that will be generated.docs/ddlog-migration-roadmap.md (1)
17-20: Reference rename looks correct
The crate name is updated tolille_ddlog, matching the refactor elsewhere..github/workflows/ci.yml (2)
15-17: GitHub Actions fallback expression – please double-check
secrets.CS_ACCESS_TOKEN || ''andvars.CODESCENE_CLI_SHA256 || ''should work, but the||operator only became valid in early-2024. If the runner is on an older Actions runtime the job will fail to parse.
36-42: Coverage upload gating looks sane
Step now runs only whenCS_ACCESS_TOKENis non-empty and uses env vars rather than direct secret references – good defence-in-depth.Makefile (1)
29-36: New path dependency – verify file really exists
generated/lille_ddlog/lib.rsis now the trigger for the DDlog targets. Make surebuild_support_runner.shactually produces this file (or that the repo already contains it); otherwise the dependent targets will be silently skipped bymake.
- Add fmt-check target for CI use that only checks Rust formatting without changes - Update fmt target to exclude generated/ directory by specifying explicit packages - Add fmt-check to .PHONY targets - Ensures generated code is not formatted while maintaining formatting checks for source code
…to build-ddlog-in-generated
- Integrate remote changes with local modifications - Keep fmt-check target usage in CI - Fix syntax issue in CODESCENE_CLI_SHA256 environment variable
- Manual updates to Makefile configuration
Remove dependency on generated/lille_ddlog/lib.rs from fmt-check target to resolve circular dependency that prevented CI from running. The fmt-check target should only verify formatting of source code in workspace packages (lille, build_support, test_utils) and not require generated DDlog code to exist.
Create generated/lille_ddlog/lib.rs.stub target that generates minimal Cargo.toml and lib.rs files to satisfy Cargo's dependency resolution during formatting operations. This allows fmt-check to run without requiring the full DDlog generation pipeline, resolving the CI failure where cargo fmt couldn't find the lille-ddlog dependency.
1 participant
Summary by Sourcery
Refactor the project to replace the ddlog_lille crate with lille_ddlog, updating build scripts, CI, and documentation to use the new package name
Enhancements:
Build:
CI:
Documentation:
Chores:
Summary by CodeRabbit