build(deps): Migrate yscope-log-viewer dependency from submodule to task-based source download. #835
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
## Walkthrough
This change removes the `yscope-log-viewer` submodule and all associated files from the repository. The `.gitmodules` entry for this submodule is deleted, and the subproject directory is removed. The dependency management for `yscope-log-viewer` in `taskfiles/deps.yaml` is refactored to use an external utility via the `yscope-dev-utils` taskfile, and the internal task for handling this dependency is eliminated. The documentation is updated to instruct users to download the log viewer source using a new task before installing dependencies.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `.gitmodules` | Removed the submodule entry for `components/log-viewer-webui/yscope-log-viewer`, unregistering it from the repository. |
| `components/log-viewer-webui/yscope-log-viewer` | Deleted the entire subproject and all its contents, removing all functionality and code related to `yscope-log-viewer`. |
| `docs/src/dev-guide/components-log-viewer-webui.md` | Updated documentation to add a setup step instructing users to run `task deps:log-viewer` before installing dependencies. |
| `taskfiles/deps.yaml` | Added `includes` for `yscope-dev-utils`. Refactored the `log-viewer` task to delegate downloading and extraction to an external utility. Removed the internal `yscope-log-viewer` task and its references from other tasks. Updated the `default` task to use `deps` for `core` and `log-viewer`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Developer
participant TaskRunner
participant yscope-dev-utils
Developer->>TaskRunner: Run task deps:log-viewer
TaskRunner->>yscope-dev-utils: remote:download-and-extract-tar (with params)
yscope-dev-utils-->>TaskRunner: Download and extract log viewer tarball
TaskRunner-->>Developer: Log viewer source readyPossibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/src/dev-guide/components-log-viewer-webui.md (1)
12-16: Documentation: Introduce log‑viewer download stepThe new setup step clearly instructs users to run
task deps:log-viewerbefore installing dependencies, aligning the guide with the updated workflow. To maintain consistency with other code fences in this doc (which useshell), consider switching the fence language:- ```bash + ```shelltaskfiles/deps.yaml (1)
71-78: Refactorlog‑viewertask to remote downloadOffloading the download-and-extract logic to
yscope-dev-utils:remote:download-and-extract-tarcentralizes dependency handling. For better maintainability, consider extracting the hard‑coded commit (969ff35) into top‑level variables:+ G_LOG_VIEWER_VERSION: "969ff35" + G_LOG_VIEWER_TAR_NAME: "yscope-log-viewer-${G_LOG_VIEWER_VERSION}.tar.gz" ... - URL: "https://github.com/y-scope/yscope-log-viewer/archive/969ff35.tar.gz" + URL: "https://github.com/y-scope/yscope-log-viewer/archive/${G_LOG_VIEWER_VERSION}.tar.gz" - TAR_FILE: "{{.G_BUILD_DIR}}/yscope-log-viewer.tar.gz" + TAR_FILE: "{{.G_BUILD_DIR}}/${G_LOG_VIEWER_TAR_NAME}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitmodules(0 hunks)components/log-viewer-webui/yscope-log-viewer(0 hunks)docs/src/dev-guide/components-log-viewer-webui.md(1 hunks)taskfiles/deps.yaml(3 hunks)
💤 Files with no reviewable changes (2)
- .gitmodules
- components/log-viewer-webui/yscope-log-viewer
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
🔇 Additional comments (2)
taskfiles/deps.yaml (2)
3-5: Delegate submodule handling to external taskfileIntroducing the
includesdirective to pull inyscope-dev-utilstasks consolidates download logic and removes duplication. Ensure that the relative path (../tools/yscope-dev-utils/exports/taskfiles/utils/utils.yaml) stays correct as the repository structure evolves.
31-33: Usedepsin default task for proper orchestrationSwitching the
defaulttask to listdeps(core,log-viewer) rather than inlinecmdsensures tasks execute in the right order and leverages Taskfile’s built‑in dependency management.
junhaoliao
requested changes
Apr 18, 2025
Member
There was a problem hiding this comment.
@coderabbitai Not related to this PR but for the "## Running" commands, we should specify in which directory the commands should be run. Create an issue to track this.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I've created an issue to track this documentation improvement. The issue describes the need to specify which directories the commands in the "Running" section should be executed from. I've assigned the issue to you as requested.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the documentation improvement
gh issue create \
--title "Documentation: Specify directories for commands in Log Viewer WebUI Running section" \
--body "## Description
In the Log Viewer WebUI documentation, the \"Running\" section does not specify which directory the commands should be run from. This makes it unclear for users whether they should run the commands from the repository root, components/log-viewer-webui, or the client/server subdirectories.
## Proposed Change
Update the \"Running\" section in \`docs/src/dev-guide/components-log-viewer-webui.md\` to specify the directory for each command, similar to how it's done in the \"Setup\" section.
## Background
This issue was identified during PR review: https://github.com/y-scope/clp/pull/835#discussion_r2049777505
" \
--assignee "junhaoliao"
Length of output: 813
@junhaoliao I've created the issue to track this documentation improvement. You can find it at: #836
The issue focuses on updating the "Running" section in the Log Viewer WebUI documentation to clearly specify which directories the commands should be executed from.
Member
|
For the PR title, how about: |
Co-authored-by: Junhao Liao <[email protected]>
kirkrodrigues
changed the title
junhaoliao
approved these changes
Apr 18, 2025
anlowee
pushed a commit
to anlowee/clp
that referenced
this pull request
Apr 25, 2025
…ask-based source download. (y-scope#835) Co-authored-by: Junhao Liao <[email protected]>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We want to move away from using submodules since they cause a few issues:
--shallow-submodules); but that's not much different than usingtaskto download a snapshot release.To address this, we've been adding tasks to yscope-dev-utils for downloading dependencies. This PR starts by replacing the yscope-log-viewer submodule with tasks to download it directly.
Checklist
breaking change.
Validation performed
Cloned a clean repo, checked out the PR branch, and then validated:
Cloned a clean repo, checked out the PR branch, and then validated:
Summary by CodeRabbit