Skip to content

Conversation

@Bill-hbrhbr
Copy link
Contributor

@Bill-hbrhbr Bill-hbrhbr commented Nov 20, 2025

Description

This PR addresses the package layout changes in #1512.

  • All binaries are removed from clp-package/bin. The directory is gone.
  • clp-package/lib is moved to build/python-libs.

Hence, bin and lib are no longer required components of the clp package.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Conforms to the current package layout.

Summary by CodeRabbit

  • Tests
    • Updated package configuration validation requirements in internal testing utilities to streamline validation checks and reduce unnecessary directory existence restrictions.

✏️ Tip: You can customize this high-level summary in your review settings.

@Bill-hbrhbr Bill-hbrhbr requested a review from a team as a code owner November 20, 2025 20:43
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Walkthrough

Modified PackageConfig validation in test utilities to require only etc and sbin directories within clp_package_dir, instead of the previous requirement for etc, sbin, bin, and lib. The missing_dirs check and corresponding error message are updated accordingly.

Changes

Cohort / File(s) Summary
Validation logic reduction
integration-tests/tests/utils/config.py
Reduced PackageConfig directory validation from four required directories (etc, sbin, bin, lib) to two (etc, sbin). Updated missing_dirs check and error messaging.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify the directory requirement reduction aligns with broader integration test or packaging strategy changes
  • Confirm error messaging remains clear and matches the new validation logic

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title clearly and specifically describes the main change: dropping obsolete bin and lib existence checks in the CLP package layout validation, which matches the PR's objective to update integration tests after package layout changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kirkrodrigues
Copy link
Member

@coderabbitai Can you open an issue corresponding to what this PR fixes?

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the problem fixed by PR #1642

gh issue create \
  --title "Integration tests fail due to outdated package layout assumptions after PR #1512" \
  --body "After PR #1512 changed the CLP package layout (removing the \`bin\` directory and moving \`lib\` to \`build/python-libs\`), the integration tests continued to check for the existence of these directories in the CLP package. This caused the tests to fail because they expected \`bin\` and \`lib\` to exist in the package, but these directories are no longer part of the package structure.

**Resolution**: The integration tests needed to be updated to reflect the new package layout by removing the existence checks for \`bin\` and \`lib\` directories.

**Backlinks**:
- Fixed in PR #1642: https://github.com/y-scope/clp/pull/1642
- Original layout change in PR #1512: https://github.com/y-scope/clp/pull/1512

**Requested by**: @kirkrodrigues" \
  --assignee kirkrodrigues

Length of output: 42


@kirkrodrigues: I've created issue #1643 to track the problem that this PR fixes. The issue describes how the integration tests failed due to outdated package layout assumptions after PR #1512 changed the structure.

Link: #1643


🧠 Learnings used
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/tests/fixtures/integration_test_logs.py:54-56
Timestamp: 2025-08-17T16:10:38.722Z
Learning: For PR #1100 (feat(integration-tests): Add CLP package integration tests boilerplate), do not raise cache weakness problems related to the pytest cache implementation in the integration test logs fixtures.

Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1100
File: integration-tests/pyproject.toml:11-13
Timestamp: 2025-08-16T10:24:29.316Z
Learning: In the CLP project's integration-tests directory, the integration tests package is not intended to be shipped or distributed - only the tests are leveraged for local testing purposes, so console script entry points should not be included in pyproject.toml.

Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Learnt from: junhaoliao
Repo: y-scope/clp PR: 1405
File: components/clp-package-utils/pyproject.toml:5-15
Timestamp: 2025-10-13T03:24:35.074Z
Learning: In the y-scope/clp repository, the Python 3.9 to 3.10 version requirement change was intentionally deferred to a separate PR (after PR #1405) to reduce review effort, as decided in an offline discussion between junhaoliao and kirkrodrigues.

@kirkrodrigues kirkrodrigues changed the title fix(integration-tests): Drop bin and lib existence checks in the CLP package layout. fix(integration-tests): Drop obsolete bin and lib existence checks in the CLP package layout (fixes #1643). Nov 20, 2025
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edited the PR title directly.

@Bill-hbrhbr Bill-hbrhbr merged commit 55044b5 into y-scope:main Nov 20, 2025
21 of 22 checks passed
@Bill-hbrhbr Bill-hbrhbr deleted the it-fix-package-required-dirs branch November 20, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants