build(taskfile): Bump emsdk version to 4.0.23; Extract emsdk and node-modules into sub-taskfiles; Change default task to package.#139
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:
WalkthroughExtracts emsdk/toolchain setup into Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Bill-hbrhbr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
taskfile.yaml (2)
37-42:⚠️ Potential issue | 🔴 CriticalUndefined variable:
G_EMSDK_CHECKSUMmoved to included file.
{{.G_EMSDK_CHECKSUM}}on line 39 is now defined intaskfiles/deps/emsdk.yaml. Task v3 variable scoping propagates variables DOWN to included files, but NOT back UP to the including file. This reference will resolve to an empty string, breaking the source tracking for theclp-ffi-jstask.🐛 Option 1: Re-declare the variable in taskfile.yaml
vars: G_BUILD_DIR: "{{.ROOT_DIR}}/build" G_CLP_FFI_JS_BUILD_DIR: "{{.G_BUILD_DIR}}/clp-ffi-js" G_DEPS_DIR: "{{.G_BUILD_DIR}}/deps" G_DIST_DIR: "{{.ROOT_DIR}}/dist" + G_EMSDK_DIR: "{{.G_BUILD_DIR}}/emsdk" + G_EMSDK_CHECKSUM: "{{.G_BUILD_DIR}}/emsdk.md5"🐛 Option 2: Remove emsdk checksum from sources (loses rebuild tracking)
sources: - "{{.G_CLP_FFI_JS_BUILD_DIR}}/CMakeCache.txt" - - "{{.G_EMSDK_CHECKSUM}}" - "{{.TASKFILE}}" - "CMakeLists.txt" - "src/**/*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfile.yaml` around lines 37 - 42, The sources entry uses the variable {{.G_EMSDK_CHECKSUM}} which is now defined only in the included file taskfiles/deps/emsdk.yaml and therefore resolves to an empty string here; fix by re-declaring G_EMSDK_CHECKSUM in the including taskfile.yaml (add a top-level variable G_EMSDK_CHECKSUM with the same value or computed expression) so the sources list can expand correctly, or if you intentionally accept losing rebuild tracking remove the "{{.G_EMSDK_CHECKSUM}}" item from the sources array; update the variable near the top of taskfile.yaml (or remove the reference) and keep the sources list that contains "{{.G_EMSDK_CHECKSUM}}" in sync with that change.
99-117:⚠️ Potential issue | 🔴 CriticalUndefined variables:
G_EMSDK_CHECKSUMandG_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE.Both variables referenced here (lines 102 and 115) are now defined only in
taskfiles/deps/emsdk.yaml. Due to Task v3's unidirectional variable scoping (parent → child only), these will be undefined:
- Line 102:
{{.G_EMSDK_CHECKSUM}}in sources will be empty- Line 115:
{{.G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE}}will pass an empty path to CMake, causing configuration failureThe
deps:emsdkdependency on line 111 ensures the emsdk is installed, but it does NOT make its variables available to this task.🐛 Proposed fix: Add required variables to taskfile.yaml vars section
vars: G_BUILD_DIR: "{{.ROOT_DIR}}/build" G_CLP_FFI_JS_BUILD_DIR: "{{.G_BUILD_DIR}}/clp-ffi-js" G_DEPS_DIR: "{{.G_BUILD_DIR}}/deps" G_DIST_DIR: "{{.ROOT_DIR}}/dist" + G_EMSDK_DIR: "{{.G_BUILD_DIR}}/emsdk" + G_EMSDK_CHECKSUM: "{{.G_BUILD_DIR}}/emsdk.md5" + G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE: >- + {{.G_EMSDK_DIR}}/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmakeNote: This creates duplication with
emsdk.yaml, but is necessary due to Task's scoping rules. Consider documenting this constraint or extracting shared vars to a common location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfile.yaml` around lines 99 - 117, The task "config-cmake-project" references variables G_EMSDK_CHECKSUM and G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE that are only defined in deps/emsdk.yaml and thus are undefined in this task; add G_EMSDK_CHECKSUM and G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE entries to the vars section of taskfile.yaml (using the same values/expressions as in taskfiles/deps/emsdk.yaml) so the sources and the -DCMAKE_TOOLCHAIN_FILE argument receive the correct values; keep names exact (G_EMSDK_CHECKSUM, G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE) to match the references in cmd and sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfiles/deps/emsdk.yaml`:
- Around line 42-44: The PATH assignment using echo on the glob
"{{.G_EMSDK_DIR}}/node/*/bin" can produce space-separated entries when multiple
matches exist, breaking PATH; update the npm install line to build a
colon-separated PATH properly (e.g., replace the echo-based expansion with a
printf/join approach or explicit single-version selection like using ls to pick
one node dir) so that PATH becomes "dir1:dir2:$PATH" even when multiple
node/*/bin matches exist; change the line that currently reads PATH=$(echo
{{.G_EMSDK_DIR}}/node/*/bin):$PATH npm install to use a colon-joining command
substitution or an explicit node directory selection.
- Around line 19-21: Replace the invalid double-colon task references in the
deps list: change "::init" to "init" and "::utils:checksum:validate" to
"utils:checksum:validate" so the task references use the supported single-colon
syntax; update the deps entries referencing "::init" and
"::utils:checksum:validate" accordingly.
---
Outside diff comments:
In `@taskfile.yaml`:
- Around line 37-42: The sources entry uses the variable {{.G_EMSDK_CHECKSUM}}
which is now defined only in the included file taskfiles/deps/emsdk.yaml and
therefore resolves to an empty string here; fix by re-declaring G_EMSDK_CHECKSUM
in the including taskfile.yaml (add a top-level variable G_EMSDK_CHECKSUM with
the same value or computed expression) so the sources list can expand correctly,
or if you intentionally accept losing rebuild tracking remove the
"{{.G_EMSDK_CHECKSUM}}" item from the sources array; update the variable near
the top of taskfile.yaml (or remove the reference) and keep the sources list
that contains "{{.G_EMSDK_CHECKSUM}}" in sync with that change.
- Around line 99-117: The task "config-cmake-project" references variables
G_EMSDK_CHECKSUM and G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE that are only defined in
deps/emsdk.yaml and thus are undefined in this task; add G_EMSDK_CHECKSUM and
G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE entries to the vars section of taskfile.yaml
(using the same values/expressions as in taskfiles/deps/emsdk.yaml) so the
sources and the -DCMAKE_TOOLCHAIN_FILE argument receive the correct values; keep
names exact (G_EMSDK_CHECKSUM, G_EMSCRIPTEN_CMAKE_TOOLCHAIN_FILE) to match the
references in cmd and sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0e63d73-950d-40c2-a1d0-9904c18929b5
📒 Files selected for processing (6)
taskfile.yamltaskfiles/deps/emsdk.yamltaskfiles/deps/main.yamltaskfiles/deps/node-modules.yamltaskfiles/lint.yamltaskfiles/test.yaml
taskfiles/deps/emsdk.yaml
Outdated
| deps: | ||
| - "::init" | ||
| - task: "::utils:checksum:validate" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Task (go-task) double colon syntax for nested includes root reference
💡 Result:
In go-task (Task), there is no double colon (::) syntax for nested includes or root references. The colon (:) separates namespaces for calling tasks from included Taskfiles. To call a task from an included Taskfile, use single colon syntax like B:up or for nested includes B:C:up (e.g., A includes B, B includes C). Root reference uses templating variables like {{.ROOT_DIR}} (absolute path of root Taskfile directory) or {{.TASKFILE_DIR}} (current/included Taskfile directory). These are available in cmds, vars, dir, etc. Colon-separated syntax (:) exists only in sources for glob patterns (experimental/recent PR #2404), e.g., sources: [".go:internal//.go"]. Official docs confirm single colon for task calls (usage, schema) and templating vars for paths (templating reference). No :: syntax found in authoritative sources.
Citations:
- 1: Vars don't get properly propagated with two levels of
includednesting go-task/task#996 - 2: TASKFILE_DIR always resolves to ROOT_DIR go-task/task#860
- 3: https://taskfile.dev/docs/guide
- 4:
includesand backward compatibility with OS specific Taskfiles go-task/task#397 - 5: https://taskfile.dev/docs/reference/templating
- 6: https://taskfile.dev/next/usage/
Replace double-colon syntax with single-colon syntax for task references.
The ::init and ::utils:checksum:validate references use invalid go-task syntax. Double colons (::) are not supported. Use single colons instead: init and utils:checksum:validate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@taskfiles/deps/emsdk.yaml` around lines 19 - 21, Replace the invalid
double-colon task references in the deps list: change "::init" to "init" and
"::utils:checksum:validate" to "utils:checksum:validate" so the task references
use the supported single-colon syntax; update the deps entries referencing
"::init" and "::utils:checksum:validate" accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfiles/toolchains.yaml`:
- Around line 12-15: The emsdk bootstrap repo is being cloned at HEAD which
makes builds non-reproducible; modify the task that clones the emsdk repository
to pin it to EMSDK_VERSION (or an explicit commit). Use the EMSDK_VERSION
variable from vars (or add EMSDK_REPO_COMMIT) and either clone with git clone
--branch "v${EMSDK_VERSION}" --depth 1 or perform git checkout
"v${EMSDK_VERSION}" (or the pinned commit) immediately after clone in the same
task/step that currently clones the repo, so the bootstrap repo matches the SDK
payload version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aeb52ac9-e2a7-46c1-8e78-2f14ae71a7de
📒 Files selected for processing (5)
taskfile.yamltaskfiles/deps.yamltaskfiles/node-modules.yamltaskfiles/test.yamltaskfiles/toolchains.yaml
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfile.yaml`:
- Around line 28-33: The clean task currently only deletes '{{.G_BUILD_DIR}}' so
running task clean leaves the top-level release artifacts in '{{.G_DIST_DIR}}'
behind; update the clean task (the clean: run: "once" cmds: list) to also remove
'{{.G_DIST_DIR}}' (e.g., add a "rm -rf '{{.G_DIST_DIR}}'" entry alongside the
existing rm -rf for '{{.G_BUILD_DIR}}'), and make the same change for the other
clean task instance referenced later in the file so both clean targets reset the
repo fully after package/build.
- Around line 6-8: The build fingerprint for the config-cmake-project task is
missing the toolchains Taskfile, so edits to toolchains aren't tracked; update
the config-cmake-project task's sources (and the other affected tasks referenced
around lines 103–105) to include the toolchains Taskfile entry (the "toolchains"
Taskfile) so Task's up-to-date check includes changes to
taskfiles/toolchains.yaml (i.e., add the toolchains Taskfile to the task's
sources/fingerprint list for config-cmake-project and the other similar tasks).
- Around line 111-113: The task has a redundant direct dependency on
"toolchains:emsdk" in the deps list; remove the "toolchains:emsdk" entry from
the deps array for the task (leave the "deps" dependency only) so that the task
relies on the existing ":toolchains:emsdk" transitively via the "deps" task;
target the deps list in taskfile.yaml (the deps key shown in the diff) and
delete the "toolchains:emsdk" line to simplify the dependency graph.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
taskfile.yaml (1)
71-77:⚠️ Potential issue | 🟠 MajorAdd package-lock.json to the
packagetask's sources to ensure cache invalidation when dependencies change.The
packagetask depends onnode-modulesbut doesn't fingerprint its own inputs (package-lock.json), meaning the build cache won't invalidate even when the dependency tree changes. This allows dist/ and the tarball to remain cached after npm dependencies update, which causes stale binaries in node_modules to be used bynpm run build:tsandnpm pack.Add the lockfile that
node-modulestracks:🛠️ Suggested fix
package: vars: CHECKSUM_FILE: "{{.G_BUILD_DIR}}/{{.TASK}}.md5" OUTPUT_DIR: "{{.G_DIST_DIR}}" sources: - "package.json" + - "package-lock.json" - "src/**/*.ts" - "tsconfig.build.json" - "tsconfig.json" - "{{.G_CLP_FFI_JS_CHECKSUM}}" - "{{.TASKFILE}}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfile.yaml` around lines 71 - 77, The package task's sources list is missing the dependency lockfile so the cache doesn't invalidate when dependencies change; update the "package" task's sources (the sources array shown) to include "package-lock.json" (the same lockfile tracked by the node-modules task) so the task fingerprints the lockfile and invalidates the cache whenever dependencies are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@taskfile.yaml`:
- Around line 40-46: The task caches for config-cmake-project and clp-ffi-js are
missing the generated dependency settings and the deps taskfile in their sources
lists; update config-cmake-project.sources and clp-ffi-js.sources to include
{{.G_DEPS_CMAKE_SETTINGS_FILE}} and taskfiles/deps.yaml so changes produced by
the deps task invalidate these tasks (ensure both entries are added to the
sources arrays alongside the existing CMake/CMakeCache and other entries).
---
Outside diff comments:
In `@taskfile.yaml`:
- Around line 71-77: The package task's sources list is missing the dependency
lockfile so the cache doesn't invalidate when dependencies change; update the
"package" task's sources (the sources array shown) to include
"package-lock.json" (the same lockfile tracked by the node-modules task) so the
task fingerprints the lockfile and invalidates the cache whenever dependencies
are updated.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
taskfiles/toolchains.yaml (1)
49-51:⚠️ Potential issue | 🟡 MinorMake the Node bin
PATHselection explicit.This still assumes
{{.G_EMSDK_DIR}}/node/*/binresolves to exactly one directory. If it ever resolves to more than one,echoturns the matches into a single space-separated string, which is not a validPATH, andnpm installcan run with the wrong toolchain.Possible fix
- |- cd "{{.G_EMSDK_DIR}}/upstream/emscripten" - PATH=$(echo {{.G_EMSDK_DIR}}/node/*/bin):$PATH npm install + PATH="$(printf '%s:' "{{.G_EMSDK_DIR}}"/node/*/bin | sed 's/:$//'):$PATH" npm install#!/bin/bash set -euo pipefail tmpdir="$(mktemp -d)" trap 'rm -rf "$tmpdir"' EXIT mkdir -p "$tmpdir/node/v1/bin" "$tmpdir/node/v2/bin" echo "echo-based PATH:" echo "$(echo "$tmpdir"/node/*/bin)" echo echo "colon-joined PATH:" printf '%s\n' "$(printf '%s:' "$tmpdir"/node/*/bin | sed 's/:$//')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@taskfiles/toolchains.yaml` around lines 49 - 51, The PATH assignment that uses PATH=$(echo {{.G_EMSDK_DIR}}/node/*/bin):$PATH before npm install can produce an invalid PATH if the glob expands to multiple dirs; replace that echo-based expansion with a deterministic colon-joined expansion (e.g. build the prefix with printf '%s:' {{.G_EMSDK_DIR}}/node/*/bin and strip the trailing colon) or explicitly pick the correct node bin (first match) and then export PATH="$PREFIX:$PATH" before running npm install; update the line that sets PATH and the subsequent npm install invocation accordingly to ensure a valid, colon-separated PATH is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@taskfiles/toolchains.yaml`:
- Around line 49-51: The PATH assignment that uses PATH=$(echo
{{.G_EMSDK_DIR}}/node/*/bin):$PATH before npm install can produce an invalid
PATH if the glob expands to multiple dirs; replace that echo-based expansion
with a deterministic colon-joined expansion (e.g. build the prefix with printf
'%s:' {{.G_EMSDK_DIR}}/node/*/bin and strip the trailing colon) or explicitly
pick the correct node bin (first match) and then export PATH="$PREFIX:$PATH"
before running npm install; update the line that sets PATH and the subsequent
npm install invocation accordingly to ensure a valid, colon-separated PATH is
used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8fce3b7-0f18-4769-a8e4-b100da91d0dc
📒 Files selected for processing (1)
taskfiles/toolchains.yaml
Bill-hbrhbr
changed the title
emsdk version to 4.0.23; Reuse shared npm workflows.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm
for the title, how about:
build(taskfile): Bump `emsdk` version to `4.0.23`; Extract emsdk and node-modules into sub-taskfiles; Change default task to `package`.
Co-authored-by: Junhao Liao <junhao@junhao.ca>
Bill-hbrhbr
changed the title
emsdk version to 4.0.23; Reuse shared npm workflows.emsdk version to 4.0.23; Extract emsdk and node-modules into sub-taskfiles; Change default task to package.
2 participants
Description
This PR contains quality-of-life improvements to the taskfile setup. The main benefit is a cleaner root
taskfile.yamlthat stays focused on top-level package build workflows, while dependency installation and package-manager setup logic are moved into dedicated sub-taskfiles.This PR also fixes the npm task wiring by reusing the existing workflows defined in
package.jsonand making task input and output paths more explicit.Checklist
breaking change.
Validation performed
Summary by CodeRabbit