fix(boost): Fix Boost task argument expansions. #103
Conversation
WalkthroughUpdated the Boost taskfile to switch command blocks from folded to literal style, remove explicit pushd/popd directory changes by declaring a working-directory context, adjust multi-line bootstrap/b2 argument formatting, and add a dir entry under the build-and-install task requirements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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 |
![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
🤖 Fix all issues with AI agents
In `@exports/taskfiles/utils/boost.yaml`:
- Around line 77-90: The Taskfile task uses dir: "{{.GENERATE_DIR}}" which makes
all commands run relative to GENERATE_DIR, so update the task's `@param`
documentation to explicitly require absolute paths for GENERATE_DIR, BUILD_DIR
and CMAKE_SETTINGS_DIR (or state that callers must pass absolute paths or they
will be resolved relative to GENERATE_DIR); mention that dir applies to all
commands (including the CMake echo) and optionally suggest callers convert
relative inputs to absolute before invoking the task so there is no silent path
resolution.
exports/taskfiles/utils/boost.yaml
Outdated
| requires: | ||
| vars: ["GENERATE_DIR", "BUILD_DIR", "INSTALL_PREFIX"] | ||
| dir: "{{.GENERATE_DIR}}" | ||
| cmds: | ||
| - >- | ||
| pushd "{{.GENERATE_DIR}}"; | ||
| ./b2 | ||
| install | ||
| --build-dir="{{.BUILD_DIR}}" | ||
| {{- range .EXTRA_ARGS}} | ||
| "{{.}}" | ||
| {{.}} | ||
| {{- end}} | ||
| {{- if .JOBS}} | ||
| "-j{{.JOBS}}" | ||
| {{- end}}; | ||
| popd; | ||
| -j{{.JOBS}} | ||
| {{- end}} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good use of dir to replace pushd/popd; note the implicit absolute-path contract.
Using dir: "{{.GENERATE_DIR}}" is cleaner and idiomatic for Taskfile. However, because dir applies to all commands in this task (including the CMake echo on line 97), every path variable used here (BUILD_DIR, CMAKE_SETTINGS_DIR) must be absolute. The current defaults derived from WORK_DIR / ROOT_DIR satisfy this, but callers providing custom relative paths would silently resolve against GENERATE_DIR. Consider documenting in the @param block that these paths are expected to be absolute.
📝 Suggested doc tweak
- # `@param` {string} BUILD_DIR Directory in which to build boost.
+ # `@param` {string} BUILD_DIR Absolute directory in which to build boost.
# `@param` {string} INSTALL_PREFIX Path prefix of where the project should be installed.
# `@param` {string[]} [EXTRA_ARGS] Any additional arguments to pass to the build command.
...
- # `@param` {string} [CMAKE_SETTINGS_DIR] If set, the directory where the project's CMake settings
+ # `@param` {string} [CMAKE_SETTINGS_DIR] If set, the absolute directory where the project's CMake settings🤖 Prompt for AI Agents
In `@exports/taskfiles/utils/boost.yaml` around lines 77 - 90, The Taskfile task
uses dir: "{{.GENERATE_DIR}}" which makes all commands run relative to
GENERATE_DIR, so update the task's `@param` documentation to explicitly require
absolute paths for GENERATE_DIR, BUILD_DIR and CMAKE_SETTINGS_DIR (or state that
callers must pass absolute paths or they will be resolved relative to
GENERATE_DIR); mention that dir applies to all commands (including the CMake
echo) and optionally suggest callers convert relative inputs to absolute before
invoking the task so there is no silent path resolution.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
lgtm! left a question
for the title, how about:
fix(boost): Fix Boost task argument expansion.
exports/taskfiles/utils/boost.yaml
Outdated
| --with-libraries={{(join "," .TARGETS)}} | ||
| {{- range .EXTRA_ARGS}} | ||
| "{{.}}" | ||
| {{.}} |
There was a problem hiding this comment.
if removing "" around the expansion can fix the issue, do we still need to remove the "extra" indent?
There was a problem hiding this comment.
see https://github.com/y-scope/yscope-dev-utils/pull/103/changes#r2777307786
i believe we should just remove the indents. the quotes should be kept
| {{.}} | |
| "{{.}}" |
exports/taskfiles/utils/boost.yaml
Outdated
| --build-dir="{{.BUILD_DIR}}" | ||
| {{- range .EXTRA_ARGS}} | ||
| "{{.}}" | ||
| {{.}} |
There was a problem hiding this comment.
in fact, if we keep the indent, i don't think simply removing the quotes is a good idea, like if i use
- >-
./b2
install
--build-dir="{{.BUILD_DIR}}"
{{- range .EXTRA_ARGS}}
{{.}}
{{- end}}
{{- if .JOBS}}
-j{{.JOBS}}
{{- end}};
with this task:
download-boost:
internal: true
vars:
BOOST_VERSION: "1.88.0"
B2_USER_CONFIG_FILE: "{{.G_DEPS_DIR}}/b2-user-config.jam"
run: "once"
deps:
- ":emsdk"
cmds:
- >-
echo 'using emscripten : : "{{.G_EMSDK_DIR}}/upstream/emscripten/emcc" ;'
> "{{.B2_USER_CONFIG_FILE}}"
- task: ":utils:boost:download-and-install"
vars:
CMAKE_SETTINGS_DIR: "{{.G_DEPS_CMAKE_SETTINGS_DIR}}"
FILE_SHA256: "85138e4a185a7e7535e82b011179c5b5fb72185bea9f59fe8e2d76939b2f5c51"
URL: "https://github.com/boostorg/boost/releases/download/boost-{{.BOOST_VERSION}}\
/boost-{{.BOOST_VERSION}}-b2-nodocs.tar.gz"
WORK_DIR: "{{.G_DEPS_DIR}}"
TARGETS: [
"headers",
"exception",
]
BUILD_AND_INSTALL_ARGS:
- "toolset=emscripten"
- "--user-config={{.B2_USER_CONFIG_FILE}}"i get
task: [utils:boost:build-and-install] ./b2 install --build-dir="/home/junhao/workspace/clp-ffi-js/build/deps/boost-build"
toolset=emscripten
--user-config=/home/junhao/workspace/clp-ffi-js/build/deps/b2-user-config.jam;
...
stat /home/junhao/workspace/clp-ffi-js/build/deps/boost-generate/--user-config=/home/junhao/workspace/clp-ffi-js/build/deps/b2-user-config.jam: no such file or directory
so i think the quotes are not an issue. the extra indent is.
we should just remove the indent then?
| {{.}} | |
| "{{.}}" |
exports/taskfiles/utils/boost.yaml
Outdated
| "-j{{.JOBS}}" | ||
| {{- end}}; | ||
| popd; | ||
| -j{{.JOBS}} |
There was a problem hiding this comment.
| -j{{.JOBS}} | |
| "-j{{.JOBS}}" |
exports/taskfiles/utils/boost.yaml
Outdated
| --with-libraries={{(join "," .TARGETS)}} | ||
| {{- range .EXTRA_ARGS}} | ||
| "{{.}}" | ||
| {{.}} |
There was a problem hiding this comment.
see https://github.com/y-scope/yscope-dev-utils/pull/103/changes#r2777307786
i believe we should just remove the indents. the quotes should be kept
| {{.}} | |
| "{{.}}" |
Bill-hbrhbr
changed the title
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
the code looks cleaner than the attempt to remove the indent, and it works perfectly with the changes in y-scope/clp-ffi-js#113
Before merging, as @Bill-hbrhbr suggested, let's wait for the approval from @davidlion
Bill-hbrhbr
changed the title
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 81
🔗 #104 The issue has been assigned to you and includes a backlink to this PR. 🧠 Learnings used |
|
@coderabbitai Why are the unit tests not running |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4125 🏁 Script executed: Length of output: 2908 🏁 Script executed: Length of output: 1984 🏁 Script executed: Length of output: 992
Here's what's happening: Why
The root cause: Solution: # Option 1: Merge main into your branch
git checkout fix-boost-option-quotes
git merge origin/main
git push
# Option 2: Rebase your branch on main (cleaner history)
git checkout fix-boost-option-quotes
git rebase origin/main
git push --force-with-leaseAfter updating your branch, the workflow files will be present and both 🧠 Learnings used |
They exist. It's just it's been some time since I've ran these workflows on my fork, they went dormant. |
This reverts commit c40b523.
|
closing and will be re-opening to trigger the stale unit-tests workflow |
3 participants
Description
This PR rewrites the Boost generate and install tasks to use the literal block scalar
|-instead of the previously used folded block scalar>-. Combined with explicit use of the line continuation character\and the command separator;, the literal block scalar preserves line breaks and spacing exactly as written, which avoids unintended loss of line breaks or whitespace. This ensures shell commands are written and interpreted more consistently within Taskfiles.Folded block scalars used inside other exported Taskfiles should also be replaced in future PRs to ensure consistency.
Checklist
breaking change.
Validation performed
clpandclp-ffi-js.Summary by CodeRabbit
(Word count: 56 words - Minimal user-facing impact.)